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

Update dependencies, especially OTEL #337

Merged
merged 1 commit into from
Jul 26, 2024
Merged

Update dependencies, especially OTEL #337

merged 1 commit into from
Jul 26, 2024

Conversation

Copy link

codecov bot commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

Comment on lines +143 to +144
'opentelemetry.instrumentation.starlette',
'opentelemetry.instrumentation.fastapi',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

) -> Meter:
with self.lock:
meter = _ProxyMeter(
self.provider.get_meter(name, version=version, schema_url=schema_url),
self.provider.get_meter(name, version=version, schema_url=schema_url, *args, **kwargs),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

instrumenting_library_version=instrumenting_library_version,
schema_url=schema_url,
)
return self.provider.get_tracer(*args, **kwargs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -146,7 +146,7 @@ def test_create_metric_gauge(metrics_reader: InMemoryMetricReader) -> None:
'data_points': [
{
'attributes': {},
'start_time_unix_nano': 0,
'start_time_unix_nano': None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexmojaki alexmojaki requested a review from Kludex July 25, 2024 16:02
@Kludex
Copy link
Member

Kludex commented Jul 26, 2024

We still support the older versions with this PR, right?

@alexmojaki
Copy link
Contributor Author

I haven't explicitly tested, but the logic should be more compatible with both past and future versions.

@Kludex
Copy link
Member

Kludex commented Jul 26, 2024

@alexmojaki do you have a proposal on how to test multiple versions of packages?

I know we have an issue about it... But is the intention to run those only on the CI?

@alexmojaki
Copy link
Contributor Author

OTEL instrumentations sometimes have multiple requirements files for tests, e.g. in https://github.com/open-telemetry/opentelemetry-python-contrib/tree/main/instrumentation/opentelemetry-instrumentation-django

They have a tox setup to use these files. I haven't managed to get it working myself and it looks very complicated. But maybe we could do something similar. If we eventually start putting lots of effort into supporting multiple versions then I think it will be very helpful to be able to simultaneously test them locally. But it's probably easier to start with just testing in CI. We can add local helpers once the workflow becomes painful.

@alexmojaki alexmojaki merged commit 54d4f3f into main Jul 26, 2024
12 checks passed
@alexmojaki alexmojaki deleted the alex/update-otel branch July 26, 2024 13:09
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.

2 participants