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

fix(opentelemetry-instrumentation-asgi): Correct http.url attribute generation #2477

Merged
merged 7 commits into from
Jul 3, 2024
Merged

fix(opentelemetry-instrumentation-asgi): Correct http.url attribute generation #2477

merged 7 commits into from
Jul 3, 2024

Conversation

dhofstetter
Copy link
Contributor

@dhofstetter dhofstetter commented Apr 29, 2024 •
edited
Loading

Description

Correct http.url and http.target attribute generation even with sub apps (fixes #2476)

  • modify the instrumentation logic
  • add unittests on starlette instrumentation
  • add unittests on fastapi instrumentation

Fixes #2476

Type of change

Please delete options that are not relevant.

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

From my point of view existing logic won't break. Simply attributes generated will change according to their correct intended values.

How Has This Been Tested?

Does This PR Require a Core Repo Change?

  • 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 (imho not required)

@dhofstetter dhofstetter requested a review from a team April 29, 2024 21:49
@dhofstetter dhofstetter force-pushed the fix/asgi-sub-apps branch 3 times, most recently from 4271c3e to ffe44f8 Compare April 29, 2024 22:11
@dhofstetter
Copy link
Contributor Author

pylint forced me to split the fastapi instrumentation tests, therefore I made a split into two files

@dhofstetter
Copy link
Contributor Author

pipeline is already running 100% green within my fork, so its safe to approve workflows

@xrmx
Copy link
Contributor

xrmx commented Apr 30, 2024

@samuelcolvin any chance you could take a look at this?

@Kludex
Copy link
Contributor

Kludex commented May 3, 2024

I'll check this today.

@xrmx
Copy link
Contributor

xrmx commented May 15, 2024

@Kludex it would be great if you can take a look at this, thanks

Kludex
Kludex approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@Kludex Kludex left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't think that amount of explanation is needed, besides a link to the docs, or issue on asgiref, but besides that... All good. 👍

@dhofstetter
Copy link
Contributor Author

I don't think that amount of explanation is needed, besides a link to the docs, or issue on asgiref, but besides that... All good. 👍

Changed the comments to prevent non required verbosity :)

@dhofstetter
Copy link
Contributor Author

Refactored as suggested and rebased to main

xrmx
xrmx reviewed Jun 27, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@dhofstetter
Copy link
Contributor Author

rebased to main :)

@lzchen
Copy link
Contributor

lzchen commented Jul 1, 2024

@dhofstetter

Could you rebase again? Seems like the pr does not allow for maintainer edits.

@dhofstetter
Copy link
Contributor Author

dhofstetter commented Jul 1, 2024 •
edited
Loading

@lzchen Rebase done

@lzchen
Copy link
Contributor

lzchen commented Jul 2, 2024

@dhofstetter

The pr always needs rebasing if a different change gets merged. I suggest allowing maintainer edits on this branch or else we will have to continuously follow up with you.

@dhofstetter
Copy link
Contributor Author

@dhofstetter

The pr always needs rebasing if a different change gets merged. I suggest allowing maintainer edits on this branch or else we will have to continuously follow up with you.

The thing is ... well tell me where to setup what, s.t. maintainers can edit :D I was/am not aware that I prevent that

@dhofstetter
fix(opentelemetry-instrumentation-asgi): Correct http.target attribut…
19dcc14 
…e generation even with sub apps (fixes open-telemetry#2476)

- modify the instrumentation logic
- add unittests on starlette instrumentation
- add unittests on fastapi instrumentation
@lzchen lzchen merged commit ef4bc9f into open-telemetry:main Jul 3, 2024
365 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzchen lzchen lzchen approved these changes

@Kludex Kludex Kludex approved these changes

@xrmx xrmx xrmx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

opentelemetry-instrumentation-asgi: root_path is handled not compliant to asgi spec
4 participants
@dhofstetter @xrmx @Kludex @lzchen