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

fix(asgi-instrumentation): extract target after running the framework #1461

Merged
merged 9 commits into from
Dec 5, 2022

Conversation

sk-
Copy link
Contributor

@sk- sk- commented Nov 19, 2022

Description

There was a mistake in the original strategy to extract the feature, as before running the framework the scope does not have all the information necessary to deduct the route.

We move the target extraction after running the framewrork function, but that means that we won't have those attributes for the span or for the active_requests metrics. The target will be available only for the duration metric.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Added a print statement to the middleware that prints duration_attrs just before sending the metric.
  • Changed unit test to reflect the fact that the scope changes after running the framework function.

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

There was a mistake in the original strategy to extract the feature, as before running the framework the scope does not have all the information necessary to deduct the route.

We move the target extraction after running the framewrork function, but that means that we won't have those attributes for the span or for the active_requests metrics. The target will be available only for the duration metric.

The test was also updated to reflect that the scope changes only after running the framework function.
@sk- sk- requested a review from a team as a code owner November 19, 2022 19:10
@sk-
Copy link
Contributor Author

sk- commented Nov 19, 2022

@srikanthccv

@srikanthccv srikanthccv enabled auto-merge (squash) November 28, 2022 12:48
srikanthccv and others added 2 commits November 30, 2022 07:02
Now the target attribute is only present in the duration metric
auto-merge was automatically disabled December 3, 2022 11:45

Head branch was pushed to by a user without write access

@sk-
Copy link
Contributor Author

sk- commented Dec 4, 2022

@srikanthccv sorry for the delay, tests are finally fixed.

@srikanthccv srikanthccv enabled auto-merge (squash) December 5, 2022 01:28
@srikanthccv srikanthccv merged commit bc57cc0 into open-telemetry:main Dec 5, 2022
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

2 participants