-
Notifications
You must be signed in to change notification settings - Fork 740
system-metrics: Add cpython.gc.collections
metric
#3617
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
base: main
Are you sure you want to change the base?
system-metrics: Add cpython.gc.collections
metric
#3617
Conversation
@@ -469,6 +470,12 @@ def _instrument(self, **kwargs: Any): | |||
description=f"Runtime {self._python_implementation} GC count", | |||
unit="By", | |||
) | |||
self._meter.create_observable_counter( | |||
name=f"process.runtime.{self._python_implementation}.gc_count", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name should be cpython.gc.collections
no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the catch. added this metrics name. this also aligns to your other comment. Now it gets its own if
@@ -469,6 +470,12 @@ def _instrument(self, **kwargs: Any): | |||
description=f"Runtime {self._python_implementation} GC count", | |||
unit="By", | |||
) | |||
self._meter.create_observable_counter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it doesn't work with pypy, can we skip for pypy from here?
Short question: Should we comment that the old metric is deprecated ? |
d4d91eb
to
4003bb0
Compare
@emdneto may you please take another look on the PR and see that all your comments were addressed? |
CHANGELOG.md
Outdated
- `opentelemetry-instrumentation-asgi`: fix excluded_urls in instrumentation-asgi | ||
([#3567](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3567)) | ||
- `opentelemetry-resource-detector-containerid`: make it more quiet on platforms without cgroups | ||
([#3579](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3579)) | ||
- `opentelemetry-instrumentation-system-metrics`: Add cpython.gc.collections metrics with collection unit is specified in semconv ([3617](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3617)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `opentelemetry-instrumentation-system-metrics`: Add cpython.gc.collections metrics with collection unit is specified in semconv ([3617](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3617)) | |
- `opentelemetry-instrumentation-system-metrics`: Add `cpython.gc.collections` metric with collection unit specified in semconv ([#3617](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3617)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can u move to the Unreleased section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cpython.gc.collections
metric
4003bb0
to
298a072
Compare
298a072
to
8a51981
Compare
@emdneto The failing test is connected to this PR. Interesting why this happens. It fails in the sqlalchemy tests which was not touched here. |
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #3549
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
The change is trivial as
opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-system-metrics/tests/test_system_metrics.py
Line 952 in b74633a
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.
I try to keep this pr as small as possible. After this is merged i want to add the cpython.gc.collected_objects and cpython.gc.uncollectable_objects metrics from https://github.com/open-telemetry/semantic-conventions/blob/2bc97890c1ad82232745b4dd74fd3144476b6a5c/docs/runtime/cpython-metrics.md#metric-cpythongccollected_objects