opentelemetry-python-contrib icon indicating copy to clipboard operation
opentelemetry-python-contrib copied to clipboard

Published 20 hours ago • verified publisher icon open-telemetry

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

Open dhofstetter opened this issue 1 year ago • 3 comments
trafficstars

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.

  • [x] 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?

  • See the corresponding bug #2476 using the apps there, and the proposed fix, it can be verified working
  • The pr contains unit tests to cover such situations as well.

Does This PR Require a Core Repo Change?

  • [x] No.

Checklist:

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

  • [x] Followed the style guidelines of this project
  • [x] Changelogs have been updated
  • [x] Unit tests have been added
  • [x] Documentation has been updated (imho not required)

dhofstetter avatar Apr 29 '24 21:04 dhofstetter

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

dhofstetter avatar Apr 29 '24 22:04 dhofstetter

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

dhofstetter avatar Apr 29 '24 22:04 dhofstetter

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

xrmx avatar Apr 30 '24 07:04 xrmx

I'll check this today.

Kludex avatar May 03 '24 07:05 Kludex

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

xrmx avatar May 15 '24 08:05 xrmx

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 avatar May 21 '24 22:05 dhofstetter

Refactored as suggested and rebased to main

dhofstetter avatar May 21 '24 22:05 dhofstetter

rebased to main :)

dhofstetter avatar Jun 28 '24 15:06 dhofstetter

@dhofstetter

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

lzchen avatar Jul 01 '24 16:07 lzchen

@lzchen Rebase done

dhofstetter avatar Jul 01 '24 21:07 dhofstetter

@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.

lzchen avatar Jul 02 '24 15:07 lzchen

@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 avatar Jul 02 '24 18:07 dhofstetter