Skip to content

Allow STDOUT overrides for AgentBasedEnvironments #147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jun 13, 2024

Conversation

PaulJSchellenberg
Copy link
Contributor

Issue #, if available: None

Description of changes:

Overview

I was looking through the docs and I noticed that if I have the awslogs log driver enabled for ECS and Fargate, I can just output the embedded metric logs to STDOUT. The only way I found to do this was to override the environment to Local and set the required dimensions with environment variables so they don't become Unknown.

Another issue with this method is that I lose all of the container context that the ECSEnvironment provides and I would like to have that.

This change introduces a new environment variable called WRITE_TO_STDOUT where if it's set to true, it will override the AgentSink and return a ConsoleSink in the AgentBasedEnvironment regardless of which AgentBasedEnvironment you use.

I went this direction because it felt like I would need to do some more potentially contentious refactoring around the environments to make an ECSStdoutEnvironment and would need to update the probe in the ECSEnvironment to verify that the cloudwatch agent is available (which I currently don't have the environment setup to test)

Lmk if you think otherwise or if you have another idea to get this done

Testing

  • unit testing
  • ran the builds

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@gordonpn
Copy link
Member

gordonpn commented Jun 6, 2024

Hi Paul, thank you for the PR.

Could you add a blurb on how this new environment variable will work, under this header on the main README: https://github.com/PaulJSchellenberg/aws-embedded-metrics-java/blob/master/README.md#configuration

Also, how it will affect the existing EnvironmentOverride?

@PaulJSchellenberg
Copy link
Contributor Author

README updated

@gordonpn gordonpn requested a review from markkuhn June 6, 2024 19:18
@gordonpn
Copy link
Member

gordonpn commented Jun 6, 2024

I've added @markkuhn as a 2nd reviewer.

Once the changes are approved, we'd like to squash merge, please (so that it appears as 1 atomic commit).

@giancarlokc giancarlokc removed the request for review from markkuhn June 12, 2024 23:24
@giancarlokc giancarlokc merged commit 2713006 into awslabs:master Jun 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants