Skip to content

Integrate with existing metrics code#432

Merged
cartersocha merged 22 commits into
open-telemetry:mainfrom
avillela:avillela-python-metrics
Oct 14, 2022
Merged

Integrate with existing metrics code#432
cartersocha merged 22 commits into
open-telemetry:mainfrom
avillela:avillela-python-metrics

Conversation

@avillela
Copy link
Copy Markdown
Contributor

@avillela avillela commented Oct 11, 2022

Fixes #72

Changes

  • Clean up requirements.txt
    • Use opentelemetry-distro to install relevant OpenTelemetry packages
    • Since we are installing psutil, opentelemetry-bootstrap will also install opentelemetry-instrumentation-system-metrics
  • Update recommendationservice's Dockerfile to:
    • Install specific version of grpcio-tools to avoid compatibility issues with protobuf library versions.
    • Install opentelemetry-bootstrap to install auto-instrumentation libraries
  • Clean up metrics and tracing init
  • Move metrics definitions to metrics.py

The following metrics are available through auto-instrumentation, courtesy of the opentelemetry-instrumentation-system-metrics, which is installed as part of opentelemetry-bootstrap on building the recommendationservice Docker image:

  • runtime.cpython.cpu_time

  • runtime.cpython.memory

  • runtime.cpython.gc_count

  • Appropriate CHANGELOG.md updated for non-trivial changes

@avillela avillela marked this pull request as ready for review October 11, 2022 20:27
@avillela avillela requested a review from a team October 11, 2022 20:27
@cartersocha
Copy link
Copy Markdown
Contributor

Please update the metric features doc under the docs folder to reflect the added feature too 😃

@cartersocha
Copy link
Copy Markdown
Contributor

I'm getting an odd error. Cursory google searching doesn't show a clear answer.

#0 13.82 × Encountered error while trying to install package.
#0 13.82 ╰─> psutil

I installed the package but still getting an error. Any special dependency requirements?

Copy link
Copy Markdown
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

I'm still not a heavy user of metrics so I'm not sure how it would be better to handle the naming.
I see here that we have cpu_usage, ram_usage.
Should we add an identifier to those metrics?

Just wondering how that will grow whenever we have 10 services with the same metric name.

Comment thread docker-compose.yml
Comment thread src/recommendationservice/metrics.py Outdated
Comment thread src/recommendationservice/recommendation_server.py
Comment thread CHANGELOG.md Outdated
Comment thread docker-compose.yml
Comment thread docker-compose.yml Outdated
Comment thread src/recommendationservice/metrics.py Outdated
Comment thread src/recommendationservice/metrics.py Outdated
Comment thread src/recommendationservice/metrics.py Outdated
Comment thread src/recommendationservice/requirements.txt Outdated
Comment thread docker-compose.yml Outdated
@mviitane
Copy link
Copy Markdown
Member

I think traces are not working with this PR without adding:
OTEL_TRACES_EXPORTER to docker-compose.yml

@fatsheep9146
Copy link
Copy Markdown
Contributor

fatsheep9146 commented Oct 12, 2022

I'm a little confused that why not using python instrumentation library to get the system metric such as cpu usage.

I'm not quite familiar about python, maybe my confusion is wrong, you can correct me. But what I really want to say is, when it come's to add new metrics or traces, we should first consider using the instrumentation library instead of manually creating one.

@reyang
Copy link
Copy Markdown
Member

reyang commented Oct 12, 2022

But what I really want to say is, when it come's to add new metrics or traces, we should first consider using the instrumentation library instead of manually creating one.

💯

@avillela
Copy link
Copy Markdown
Contributor Author

I'm a little confused that why not using python instrumentation library to get the system metric such as cpu usage.

I'm not quite familiar about python, maybe my confusion is wrong, you can correct me. But what I really want to say is, when it come's to add new metrics or traces, we should first consider using the instrumentation library instead of manually creating one.

That was me just messing around with metrics and trying to understand them, and I forgot to remove the unnecessary stuff.

@avillela
Copy link
Copy Markdown
Contributor Author

I think traces are not working with this PR without adding:
OTEL_TRACES_EXPORTER to docker-compose.yml

I was able to send traces without issue...

@avillela
Copy link
Copy Markdown
Contributor Author

I'm still not a heavy user of metrics so I'm not sure how it would be better to handle the naming. I see here that we have cpu_usage, ram_usage. Should we add an identifier to those metrics?

Just wondering how that will grow whenever we have 10 services with the same metric name.

That waste messing around with learning metrics. I'd forgotten to remove these.

@avillela avillela requested review from julianocosta89 and mviitane and removed request for mviitane October 12, 2022 18:39
Copy link
Copy Markdown
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

Great one!

Comment thread src/recommendationservice/recommendation_server.py
@avillela avillela requested a review from mviitane October 12, 2022 21:24
Comment thread src/recommendationservice/metrics.py Outdated
@mviitane
Copy link
Copy Markdown
Member

mviitane commented Oct 13, 2022

I think traces are not working with this PR without adding:
OTEL_TRACES_EXPORTER to docker-compose.yml

I was able to send traces without issue...

OK, then it was something in my environment. It seems otlp is the default.

I actually think it makes sense to set both of these env variables or then none of them (just for consistency, I believe the end result will be the same):

OTEL_TRACES_EXPORTER=otlp
OTEL_METRICS_EXPORTER=otlp

@avillela avillela requested review from fatsheep9146 and mviitane and removed request for fatsheep9146 and mviitane October 13, 2022 14:41
Copy link
Copy Markdown
Member

@mviitane mviitane left a comment

Choose a reason for hiding this comment

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

LGTM

@fatsheep9146
Copy link
Copy Markdown
Contributor

https://github.com/open-telemetry/opentelemetry-demo/blob/main/docs/metric_service_features.md

I think this file can also be updated, since Recommendation service auto instrumentation is done.

@avillela
Copy link
Copy Markdown
Contributor Author

avillela commented Oct 13, 2022

https://github.com/open-telemetry/opentelemetry-demo/blob/main/docs/metric_service_features.md

I think this file can also be updated, since Recommendation service auto instrumentation is done.

Added @fatsheep9146

@avillela avillela requested a review from fatsheep9146 October 13, 2022 16:31
Comment thread docs/services/recommendationservice.md Outdated
Comment thread docs/services/recommendationservice.md
Comment thread docs/services/recommendationservice.md
Juliano Costa and others added 4 commits October 13, 2022 22:20
@avillela avillela requested review from julianocosta89 and removed request for fatsheep9146 October 13, 2022 20:27
Comment thread docs/services/recommendationservice.md Outdated
@austinlparker
Copy link
Copy Markdown
Member

is the requested change actually blocking anything? this needs to merge today to get into 0.6

@cartersocha cartersocha merged commit 672387c into open-telemetry:main Oct 14, 2022
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
mohamed3637 added a commit to mohamed3637/opentelemetry-demo that referenced this pull request Oct 7, 2025
cloud-hb pushed a commit to cloud-hb/opentelemetry-demo that referenced this pull request Nov 17, 2025
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.

Review and enhance metric support for recommendation service (Python)

7 participants