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

refactor(instr-fetch): move fetch to use SEMATRR #4632

Merged
merged 5 commits into from
Jun 8, 2024

Conversation

mmouru
Copy link
Contributor

@mmouru mmouru commented Apr 14, 2024

Which problem is this PR solving?

Updates #4567

Short description of the changes

  • Updates deprecated SemanticAttributes in experimental package instrumentation-fetch

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

  • npm test

Checklist:

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

@mmouru mmouru requested a review from a team as a code owner April 14, 2024 14:53
Copy link

linux-foundation-easycla bot commented Apr 14, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@david-luna
Copy link
Contributor

Hi @mmouru

thanks for contributing into our effort to update semantic conventions. 🎉

Would you please complete this PR by adding a section in the README file listing the attributes used in this instrumentation?

You can have a look at intrumentation-http README

@david-luna
Copy link
Contributor

Thank you @mmouru for your contribution :)

Would you please add a section in the README file listing the attributes used in this instrumentation? Here is an example (you can omit the 3rd column) https://github.com/open-telemetry/opentelemetry-js/tree/main/experimental/packages/opentelemetry-instrumentation-http#semantic-conventions

@mmouru mmouru force-pushed the fetch-semconv branch 6 times, most recently from 17240f3 to 301a207 Compare May 7, 2024 17:47
@mmouru
Copy link
Contributor Author

mmouru commented May 7, 2024

done my bits

experimental/CHANGELOG.md Outdated Show resolved Hide resolved
@JamieDanielson
Copy link
Member

JamieDanielson commented Jun 3, 2024

Oh. @mmouru I've just realized you haven't signed the CLA. Can you please take care of that so we can move forward with this PR? fixed

@JamieDanielson
Copy link
Member

JamieDanielson commented Jun 3, 2024

Oh. @mmouru I've just realized you haven't signed the CLA. Can you please take care of that so we can move forward with this PR?

🤔 Looks like you've contributed before; there was a problem for a short time with EasyCLA, try adding a comment that says /easycla and that might fix it.

EDIT never mind, looks like I was able to fix it with this comment 😃

@JamieDanielson
Copy link
Member

/easycla

update readme table to use attribute strings
@JamieDanielson
Copy link
Member

I'm not sure why the rest of the status checks / tests are not showing up right now 🤔

@trentm
Copy link
Contributor

trentm commented Jun 6, 2024

I'm not sure why the rest of the status checks / tests are not showing up right now 🤔

Conflict in the CHANGELOG file. I'm not sure if that is what blocks running the other checks.

@JamieDanielson
Copy link
Member

Conflict in the CHANGELOG file. I'm not sure if that is what blocks running the other checks.

Ah looks like you're right. For some reason I was thinking that wouldn't block tests. I've just pushed up a merge and update to remove the conflict and the rest of the checks are running now.

Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

@trentm I've gone ahead and made the updates on this PR. Can you take a look to confirm that the updates have been addressed, and approve / dismiss the review requesting changes?

@trentm trentm added this pull request to the merge queue Jun 8, 2024
Merged via the queue into open-telemetry:main with commit 2e42181 Jun 8, 2024
19 checks passed
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

5 participants