Skip to content
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

Follow upon cloudwatch formatters #138

Conversation

terencehonles
Copy link
Contributor

This PR brings #117 up to date and addresses the backwards incompatibility mentioned here #117 (comment)

@@ -219,7 +292,7 @@ def emit(self, message):
if stream_name not in self.sequence_tokens:
self.sequence_tokens[stream_name] = None

if isinstance(message.msg, Mapping):
if self.json_serialize_default and isinstance(message.msg, Mapping):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dumping JSON will only happen once so it is fine to leave this here instead of removing it.

Copy link
Owner

@kislyuk kislyuk left a comment

Choose a reason for hiding this comment

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

This looks good overall, but it needs tests for CloudWatchJSONFormatter. Also, please replace all occurrences of "CloudWatch" with "CloudWatchLog". The former is a different AWS product that is only vaguely related to CWL, so we want to avoid confusion there.

self.fields = fields

def format(self, message):
if self.fields == '__all__':
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove the magic value "all" and use None as the placeholder for keeping all field values instead.

@kislyuk
Copy link
Owner

kislyuk commented Jan 22, 2021

@terencehonles I can add you as a collaborator in this repo if you want CI tests to run correctly (secrets can't be provided to forks). Do you want me to do that?

@terencehonles
Copy link
Contributor Author

@terencehonles I can add you as a collaborator in this repo if you want CI tests to run correctly (secrets can't be provided to forks). Do you want me to do that?

Sure that's fine.

@terencehonles
Copy link
Contributor Author

Actually that might require me to create a new PR since I think that would require this PR come from a branch in this repo. I'm not in a rush for these changes and not sure if it might make sense to check how the credentials are locked down. Not sure if you plan on changing the settings to allow forks once you've done that. If you're not comfortable with that we should probably change the tests to use mocks.

@kislyuk
Copy link
Owner

kislyuk commented Jan 30, 2021

@terencehonles are you planning to update this PR for the feedback above? Your changes look good overall, the PR just needs those minor improvements to be merged.

@terencehonles
Copy link
Contributor Author

I'll try to tend to this when I can. I bumped #144 and made it more useful by actually running mypy (instead of just using it for Sphinx docs), but I'll probably want to try to focus on #139 since I've had to remove this library in order to allow uWSGI to reload gracefully which I hadn't really noticed it wasn't doing until I increased the reload timeout and noticed it was using the full timeout much more often than I would have expected.

@kislyuk
Copy link
Owner

kislyuk commented Nov 14, 2021

Thank you for your effort on this PR and for the pointers to formatter best practices. I have implemented the relevant logic in #158 and will release it in v2.0.0. I'm going to close this PR since it is not compatible with some of the changes made in #158.

@kislyuk kislyuk closed this Nov 14, 2021
@terencehonles terencehonles deleted the follow-upon-cloudwatch-formatters branch November 14, 2021 08:48
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