Skip to content

Add LocalEnvironment using stdout #61

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 1 commit into from
Jan 14, 2021
Merged

Add LocalEnvironment using stdout #61

merged 1 commit into from
Jan 14, 2021

Conversation

02strich
Copy link
Contributor

@02strich 02strich commented Jan 6, 2021

So far the DefaultEnvironment was hard coded to use the agent sink as that
was the recommendation for workloads on-premise.

While a fair recommendation, it isn't accurate for all workloads. This
adds a similer local environment that uses the StdoutSink instead of the agent sink
for such customers.

Issue #, if available:

Description of changes:

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

Copy link
Member

@jaredcnance jaredcnance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I'll need to take a closer look at this for breaking changes. 1 other comment below.

@@ -17,14 +17,15 @@
from aws_embedded_metrics.serializers.log_serializer import LogSerializer


class LambdaSink(Sink):
class StdoutSink(Sink):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

if self.sink is None:
self.sink = AgentSink(self.get_log_group_name(), Config.log_stream_name)
return self.sink


class DefaultStdoutEnvironment(DefaultEnvironment):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the other implementations, we provide a LocalEnvironment for this purpose. Would that meet your needs here? Any particular reason to depart from that pattern?

https://github.com/awslabs/aws-embedded-metrics-node/blob/master/src/environment/LocalEnvironment.ts#L58

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I had not seen that (my need is python-only). My thinking was to maximize code re-use as DefaultEnvironment is almost exactly what was needed. I can change it to call it LocalEnvironment to be consistent though 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So made this change and now the test is happy as well :)

So far the DefaultEnvironment was hard coded to use the agent sink as that
was the recommendation for workloads on-premise.

While a fair recommendation, it isn't accurate for all workloads. This
adds a similer local environment that uses the StdoutSink instead of the agent sink
for such customers.
@02strich 02strich changed the title Add DefaultEnvironment using stdout Add LocalEnvironment using stdout Jan 12, 2021
@02strich
Copy link
Contributor Author

@jaredcnance if you got some time to have another look would be great

@jaredcnance jaredcnance merged commit dbdfb9e into awslabs:master Jan 14, 2021
@hussam789
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
General
Ensure output is flushed

The accept method should flush stdout after printing to ensure metrics are
immediately visible, especially in containerized environments where output
buffering can occur.

aws_embedded_metrics/sinks/stdout_sink.py [24-27]

 def accept(self, context: MetricsContext) -> None:
     for serialized_content in self.serializer.serialize(context):
         if serialized_content:
-            print(serialized_content)
+            print(serialized_content, flush=True)
  • Apply this suggestion
Suggestion importance[1-10]: 5

__

Why: The change to add flush=True to the print statement helps to ensure immediate output, which is beneficial in buffered or containerized environments, representing a modest but useful improvement.

Low
  • More

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