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.

Sign up for GitHub

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

Jump to bottom

add OTEL_PYTHON_HTTP_SERVER_REQUEST_DURATION_SERVER_ATTRIBUTES_ENABLED flag #2597

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

add OTEL_PYTHON_HTTP_SERVER_REQUEST_DURATION_SERVER_ATTRIBUTES_ENABLED flag #2597

wants to merge 12 commits into from

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Jun 11, 2024 •
edited
Loading

Description

add

  • OTEL_PYTHON_HTTP_SERVER_REQUEST_DURATION_SERVER_ATTRIBUTES_ENABLED flag to add server.address and server.port for http.server.request.duration - which are opt-in
  • OTEL_PYTHON_HTTP_SERVER_ACTIVE_REQUESTS_COUNT_SERVER_ATTRIBUTES_ENABLED - flag to add server.address and server.port for http.server.active_requests - also opt-in

This env var does not exist for any other language so far.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

tests included in this PR

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@zeitlinger zeitlinger requested a review from a team June 11, 2024 16:32
@github-actions github-actions bot requested a review from ocelotl June 11, 2024 16:33
@zeitlinger zeitlinger mentioned this pull request Jun 17, 2024
Closed
@zeitlinger zeitlinger force-pushed the http-metrics-server-arributes branch from 2f53be4 to 1841e73 Compare August 27, 2024 12:43
@github-actions github-actions bot requested a review from shalevr August 27, 2024 12:44
@zeitlinger zeitlinger changed the title add OTEL_ENABLE_SERVER_ATTRIBUTES_FOR_REQUEST_DURATION flag add OTEL_PYTHON_HTTP_SERVER_REQUEST_DURATION_SERVER_ATTRIBUTES_ENABLED flag Aug 27, 2024
@zeitlinger zeitlinger force-pushed the http-metrics-server-arributes branch from 1841e73 to 9662dbc Compare August 27, 2024 15:36
@zeitlinger
Copy link
Member Author

@xrmx can you check again?

emdneto
emdneto reviewed Sep 3, 2024
View reviewed changes
Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

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

Thanks for working on this one. Left some comments

...nstrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py Outdated Show resolved Hide resolved
instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py Outdated Show resolved Hide resolved
instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py Outdated Show resolved Hide resolved
opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py Outdated Show resolved Hide resolved
opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py Outdated

_OpenTelemetrySemanticConventionStability.server_duration_attrs_new_effective = (
_server_duration_attrs_new_with_server_attributes
if os.environ.get(
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to run this for all instrumentation migrated to new semconv, right? because not every instrumentation has metrics implemented. Wdyt we have a way to explicitly initialize this per instrumentation we want to support the opt-in attributes?

Copy link
Member Author

Choose a reason for hiding this comment

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

this field is only used for metrics here - maybe rename the field?

https://github.com/zeitlinger/opentelemetry-python-contrib/blob/http-metrics-server-arributes/opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py#L223

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to supporting this individually per instrumentation. _OpenTelemetrySemanticConventionStability should only be used for semantic convention migration. We do not want to mix this functionality with semantic conventions.

Copy link
Member Author

Choose a reason for hiding this comment

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

is this going in the right direction?

https://github.com/zeitlinger/opentelemetry-python-contrib/compare/http-metrics-server-arributes...zeitlinger:opentelemetry-python-contrib:http-metrics-server-arributes-asgi-trial?expand=1

Copy link
Contributor

Choose a reason for hiding this comment

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

@zeitlinger

Yeah you mostly like don't need to store the config as part of the global _OpenTelemetrySemanticConventionStability . I think you can store the config information in each instrumentation instance themselves when creating them. You can then have a static set of attributes with the one without the flag enabled and one with the flag enabled.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sympathetic to the @lzchen approach. With that we can be more selective in which instrumentation we want to support to opt-in attributes

Copy link
Member Author

Choose a reason for hiding this comment

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

ok - changed now 😄

@ocelotl ocelotl removed their assignment Sep 3, 2024
@zeitlinger
Copy link
Member Author

@emdneto please have another look 😄

lzchen
lzchen reviewed Sep 4, 2024
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
xrmx
xrmx reviewed Sep 11, 2024
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
@zeitlinger
Copy link
Member Author

feedback from SIG meeting

  • Should not add new environment variables - opens up too many possibilities (and file config will take care of that)
  • But it should be possible to do programmatically (or a PR to allow this would be welcome)

@xrmx
Copy link
Contributor

xrmx commented Sep 13, 2024 •
edited
Loading

Maybe I am late but would adding a single environment variable that takes a list of additional attributes to export would be more acceptable? The caveat would be to require specifying the accepted literals with types so at least it's clear what the code supports.
I understand the concern for avoiding too many environment variables but I'm sympathetic to the auto-instrumentation cause 😅

@emdneto
Copy link
Member

emdneto commented Sep 13, 2024 •
edited
Loading

@xrmx That's the suggestion I made at SIG yesterday. I don't know if my explanation was very clear, but we ended up discussing and agreed to do that per instrumentation using the kwargs at the instrument level.

@lzchen
Copy link
Contributor

lzchen commented Sep 16, 2024

@xrmx

I'm open to having a separate discussion about what efforts we should make for auto instrumentation to not block this PR. Seems like we have enough interest for autoinstrumentation that we should definitely do an audit of it.

lzchen
lzchen reviewed Sep 16, 2024
View reviewed changes
instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py
OTEL_PYTHON_HTTP_SERVER_ACTIVE_REQUESTS_COUNT_SERVER_ATTRIBUTES_ENABLED: "1",
},
)
def test_basic_metric_success_new_semconv_server_address(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be able to add tests for django, asgi and wsgi as well?

lzchen
lzchen requested changes Sep 16, 2024
View reviewed changes
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

Blocking until we remove env var mechanism.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xrmx xrmx xrmx left review comments

@emdneto emdneto emdneto left review comments

@lzchen lzchen lzchen requested changes

@ocelotl ocelotl Awaiting requested review from ocelotl

@shalevr shalevr Awaiting requested review from shalevr

Requested changes must be addressed to merge this pull request.

Assignees

@ocelotl ocelotl

@shalevr shalevr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zeitlinger @xrmx @emdneto @lzchen @ocelotl @shalevr