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

chore: Lock OpenTelemetry versions and add tracing metadata #2928

Merged
merged 13 commits into from
Aug 23, 2022

Conversation

ssheng
Copy link
Collaborator

@ssheng ssheng commented Aug 21, 2022

What does this PR address?

Following #2918, verified that the latest OpenTelemetry version 0.33b0 is compatible with the existing otlp, jaeger and zipkin exporters. Locking the dependency versions until OTEL stabilizes.

In addition, this PR also configures the OpenTelemetry resource automatically if user has not explicitly configured it through environment variables.

  • API servers and runners are treated as OpenTelemetry services and named after their names.
  • Service namespace is set as the Bento name
  • Service version is set as the Bento version
  • Service instance ID is set as the worker ID

Individual trace page on Jaeger. API server and runners are labeled as separate services.
Screen Shot 2022-08-21 at 02 54 17

Search page:
Screen Shot 2022-08-21 at 03 00 02

Before submitting:

Who can help review?

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

@ssheng ssheng requested a review from a team as a code owner August 21, 2022 10:42
@ssheng ssheng requested review from parano and removed request for a team August 21, 2022 10:42
@pep8speaks
Copy link

pep8speaks commented Aug 21, 2022

Hello @ssheng, Thanks for updating this PR.

There are currently no PEP 8 issues detected in this PR. Cheers! 🍻

Comment last updated at 2022-08-22 08:35:54 UTC

@codecov
Copy link

codecov bot commented Aug 21, 2022

Codecov Report

Merging #2928 (77f6af3) into main (2552c8b) will increase coverage by 0.05%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2928      +/-   ##
==========================================
+ Coverage   69.80%   69.86%   +0.05%     
==========================================
  Files         121      121              
  Lines        9930     9954      +24     
==========================================
+ Hits         6932     6954      +22     
- Misses       2998     3000       +2     
Impacted Files Coverage Δ
bentoml/_internal/log.py 82.45% <83.33%> (-0.16%) ⬇️
bentoml/_internal/configuration/containers.py 78.50% <100.00%> (+1.97%) ⬆️
bentoml/_internal/context.py 84.76% <100.00%> (-0.70%) ⬇️
bentoml/_internal/utils/circus/watchfilesplugin.py 88.15% <100.00%> (ø)

@ssheng ssheng requested a review from bojiang August 21, 2022 10:52
Comment on lines 100 to 104
result: str = (
component_context.component_type
if component_context.component_type is not None
else ""
)
Copy link
Member

Choose a reason for hiding this comment

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

imo separating these into two lines are better for better clearance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not exactly sure if I understood the suggestion. Would you like to add a suggestion and I will commit.

bentoml/_internal/log.py Outdated Show resolved Hide resolved
Co-authored-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
@ssheng ssheng merged commit 45ccd15 into bentoml:main Aug 23, 2022
@ssheng ssheng deleted the tracing-metadata branch August 23, 2022 04:16
@dbuades
Copy link
Contributor

dbuades commented Aug 23, 2022

@ssheng I'm late to the party but it looks good to me!

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

4 participants