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

feat: logging customization #2961

Merged
merged 15 commits into from Sep 12, 2022
Merged

Conversation

jiewpeng
Copy link
Contributor

@jiewpeng jiewpeng commented Sep 1, 2022

What does this PR address?

Adds customisation for logging, for example to change the format of the opentelemetry trace id and span id. Thread: https://bentoml.slack.com/archives/CKRANBHPH/p1661766094533779

Before submitting:

Who can help review?

Feel free to tag members/contributors who can help review your PR.

@jiewpeng jiewpeng requested a review from a team as a code owner September 1, 2022 02:22
@jiewpeng jiewpeng requested review from jjmachan and removed request for a team September 1, 2022 02:22
@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Merging #2961 (021495c) into main (2d03474) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2961      +/-   ##
==========================================
+ Coverage   70.60%   70.63%   +0.03%     
==========================================
  Files         104      104              
  Lines        9539     9550      +11     
==========================================
+ Hits         6735     6746      +11     
  Misses       2804     2804              
Impacted Files Coverage Δ
bentoml/_internal/configuration/containers.py 78.89% <100.00%> (+0.39%) ⬆️
bentoml/_internal/log.py 87.50% <100.00%> (+1.53%) ⬆️

@jiewpeng
Copy link
Contributor Author

jiewpeng commented Sep 1, 2022

I'm open to discussion on the following

  • Default trace_id and span_id format: currently I am proposing to change the default format to hexadecimal, which will be different from bentoml's current behaviour (logging as integer).
  • Exposing the logging formatting for the user to configure. I think this is useful for organizations that want to standardise their logging formats across their apps.

Copy link
Member

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

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

LGTM, with some minor comments.

@@ -535,5 +541,12 @@ def duration_buckets(
)
return DEFAULT_BUCKET

@providers.SingletonFactory
@staticmethod
def logging_formats(
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to make this logging_formatting to be consistent with the config level.

Copy link
Contributor Author

@jiewpeng jiewpeng Sep 5, 2022

Choose a reason for hiding this comment

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

I could change it, but would there be a use case for someone to have one trace id format for the api server and a different format for the runner? When I requested for this feature, the premise was that I would want the trace id format to be consistent across my applications, so I don't really see a use case where a user would want different formats.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I was saying just to change the naming to make it consistent

bentoml/_internal/log.py Outdated Show resolved Hide resolved
bentoml/_internal/log.py Outdated Show resolved Hide resolved
@jiewpeng
Copy link
Contributor Author

jiewpeng commented Sep 6, 2022

Have done the import lazily, but I'm just curious, is there a reason to do so. given it will run at some point anyway, provided the model is called? Based on my understanding, this delays the import of the BentoMLContainer for as long as possible, until the model gets invoked. Either this import is slow, in which case doing the import eagerly slows down the first request to the model, or the import is fast, and there is no difference (or it makes logging slightly slower because we always run the 3 lines to get the trace/span id format every time we log something, when those things do not change).

@aarnphm
Copy link
Member

aarnphm commented Sep 6, 2022

Have done the import lazily, but I'm just curious, is there a reason to do so.

This has to do with importing bentoml to global namespace is actually pretty inefficient. We will fix this some day 😄

logging_formatting is a singletonFactory, meaning it will be cached once, so actually the access for the field are pretty fast, so I'm not too worried about the performance deficit if we do it lazily.

@aarnphm
Copy link
Member

aarnphm commented Sep 7, 2022

can you rebase to the main branch, I saw there were some bad commits there. Otherwise this is GTG

creativedutchmen and others added 4 commits September 12, 2022 23:30
YAML fields for disabling access logs on docs is not correct before. This PR address this.
## What does this PR address?
<!--
Thanks for sending a pull request!

Congrats for making it this far! Here's a 🍱 for you. There are still a
few steps ahead.

Please make sure to read the contribution guidelines, then fill out the
blanks below before requesting a code review.

Name your Pull Request with one of the following prefixes, e.g. "feat:
add support for PyTorch", to indicate the type of changes proposed. This
is based on the [Conventional Commits
specification](https://www.conventionalcommits.org/en/v1.0.0/#summary).
  - feat: (new feature for the user, not a new feature for build script)
  - fix: (bug fix for the user, not a fix to a build script)
  - docs: (changes to the documentation)
- style: (formatting, missing semicolons, etc; no production code
change)
  - refactor: (refactoring production code, eg. renaming a variable)
  - perf: (code changes that improve performance)
- test: (adding missing tests, refactoring tests; no production code
change)
  - chore: (updating grunt tasks etc; no production code change)
- build: (changes that affect the build system or external dependencies)
  - ci: (changes to configuration files and scripts)
  - revert: (reverts a previous commit)

Describe your changes in detail. Attach screenshots here if appropriate.

Once you're done with this, someone from BentoML team or community
member will help review your PR (see "Who can help review?" section for
potential reviewers.). If no one has reviewed your PR after a week have
passed, don't hesitate to post a new comment and ping @-the same person.
Notifications sometimes get lost 🥲.
-->

<!-- Remove if not applicable -->
Fixes #(issue)

## Before submitting:
<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!--- If you plan to update documentation or tests in follow-up, please
note -->
- [ ] Does the Pull Request follow [Conventional Commits
specification](https://www.conventionalcommits.org/en/v1.0.0/#summary)
naming? Here are [GitHub's

guide](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request)
on how to create a pull request.
- [ ] Does the code follow BentoML's code style, both `make format` and
`make lint` script have passed
([instructions](https://github.com/bentoml/BentoML/blob/main/DEVELOPMENT.md#style-check-auto-formatting-type-checking))?
- [ ] Did you read through [contribution
guidelines](https://github.com/bentoml/BentoML/blob/main/CONTRIBUTING.md#ways-to-contribute)
and follow [development
guidelines](https://github.com/bentoml/BentoML/blob/main/DEVELOPMENT.md#start-developing)?
- [ ] Did your changes require updates to the documentation? Have you
updated
those accordingly? Here are [documentation
guidelines](https://github.com/bentoml/BentoML/tree/main/docs) and [tips
on writting
docs](https://github.com/bentoml/BentoML/tree/main/docs#writing-documentation).
- [ ] Did you write tests to cover your changes?

## Who can help review?

Feel free to tag members/contributors who can help review your PR.
<!--
Feel free to ping any of the BentoML members for help on your issue, but
don't ping more than three people 😊.
If you know how to use git blame, that is probably the easiest way.

Team members that you can ping:
- @parano
- @yubozhao
- @bojiang
- @ssheng
- @aarnphm
- @sauyon
- @larme
- @yetone
- @jjmachan
-->
Copy link
Contributor

@sauyon sauyon left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@sauyon sauyon merged commit fcd518e into bentoml:main Sep 12, 2022
sauyon added a commit to sauyon/BentoML that referenced this pull request Sep 16, 2022
Adds customization for the opentelemetry trace id and span id formats.

Also changes the default format to hexadecimal. This is the standard format of
trace and span IDs, and while it may break some workflows, hopefully it should
be very minor.

Co-authored-by: Jinsung Lee <goflrkqk@naver.com>
Co-authored-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Co-authored-by: Sauyon Lee <2347889+sauyon@users.noreply.github.com>
Co-authored-by: Benjamin Tan Wei Hao <benjamintanweihao@gmail.com>
Co-authored-by: Huib Keemink <huib.keemink@creativedutchmen.com>
Co-authored-by: Sean Sheng <s3sheng@gmail.com>
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.

None yet

7 participants