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.

Sign up for GitHub

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

Jump to bottom

Add Global context fields to StdlibFormatter #65

Merged
merged 7 commits into from
Oct 27, 2021
Merged

Add Global context fields to StdlibFormatter #65

merged 7 commits into from
Oct 27, 2021

Conversation

rdifrango
Copy link
Contributor

Resolves #57

Please note; I'm a Python newbie so if there are best practices, etc that I missed feedback is welcomed.

I did run black to format the affected files.

@cla-checker-service
Copy link

cla-checker-service bot commented Oct 7, 2021 •
edited
Loading

💚 CLA has been signed

@apmmachine
Copy link
Contributor

apmmachine commented Oct 7, 2021 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-10-27T17:31:26.840+0000

  • Duration: 6 min 47 sec

  • Commit: 1120ec0

Test stats 🧪

Test Results
Failed 0
Passed 165
Skipped 0
Total 165

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@rdifrango
Update _stdlib.py
fa86fd6 
Updated the type hints.
basepi
basepi approved these changes Oct 12, 2021
View reviewed changes
Copy link
Contributor

@basepi basepi 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 awesome, thank you!

@basepi
Copy link
Contributor

basepi commented Oct 12, 2021

/test

@basepi
Copy link
Contributor

basepi commented Oct 12, 2021

@rdifrango Can you please sign the CLA? Thanks!

@basepi basepi changed the title Resolves #57 Add Global context fields to StdlibFormatter Oct 12, 2021
@basepi
Copy link
Contributor

basepi commented Oct 12, 2021 •
edited
Loading

I fixed the lint issues -- for future reference you could check out pre-commit -- that would have run mypy and flake8 as well as black to make sure everything was done correctly.

That said, this was an excellent PR for a first-time contributor. You even added a test! Thank you!

@rdifrango
Copy link
Contributor Author

@basepi - Thank you for the feedback and yes I wasn't going to make a change without a test that's simply not how I roll :)

I should have thought to use pre-commit but I'm new to it as well. Perhaps a small set of steps on how to run to run those might be helpful?

Should I update to the latest master?

Oh and @basepi on the CLA, I'm awaiting our legal department to approve it. The OSS office which happens to be on the same team as me is trying to push it through.

@basepi
Copy link
Contributor

basepi commented Oct 12, 2021

I'm awaiting our legal department to approve it.

Excellent. Ping me when it's complete and we'll merge in master and get this merged.

Perhaps a small set of steps on how to run to run those might be helpful?

brew install pre-commit
pre-commit install  # while in the repo you want to use it in

That should make it so the next time you commit, it sets up and runs pre-commit. It's very nice!

@rdifrango
Copy link
Contributor Author

@basepi - finally got our legal team to approve the CLA and our OSS lead @cpolanec submitted it for approval by Elastic.

@basepi
Copy link
Contributor

basepi commented Oct 27, 2021

@rdifrango great news! We'll get this merged.

sethmlarson
sethmlarson reviewed Oct 27, 2021
View reviewed changes
Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

One question, otherwise this looks good.

ecs_logging/_stdlib.py Outdated
style="%", # type: str
validate=None, # type: Optional[bool]
stack_trace_limit=None, # type: Optional[int]
extra=None, # type: Optional[Dict[Any, Any]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be Optional[Dict[str, Any]]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sethmlarson - That's a good point, @basepi should I change it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rdifrango Yes please!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, now let's hope it builds ;)

sethmlarson
sethmlarson approved these changes Oct 27, 2021
View reviewed changes
Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

LGTM

@basepi basepi merged commit 7a4802c into elastic:master Oct 27, 2021
@basepi
Copy link
Contributor

basepi commented Oct 27, 2021

Don't know why the docs build was stuck but this doesn't touch docs so I merged it. Thanks again for the contribution!

@rdifrango rdifrango mentioned this pull request May 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@basepi basepi basepi approved these changes

@sethmlarson sethmlarson sethmlarson approved these changes

Assignees
No one assigned
Labels
agent-python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Global context fields when creating ecs_logging.StdlibFormatter
4 participants
@rdifrango @apmmachine @basepi @sethmlarson