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

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

Merged
merged 5 commits into from
Jun 8, 2024
Merged

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

merged 5 commits into from
Jun 8, 2024

Conversation

pahiz
Copy link
Contributor

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

@pahiz pahiz requested a review from a team April 14, 2024 14:53
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Apr 14, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@pahiz pahiz mentioned this pull request Apr 14, 2024
Closed
24 tasks
@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

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

pahiz commented May 7, 2024

done my bits

trentm
trentm requested changes May 14, 2024
View reviewed changes
experimental/CHANGELOG.md Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-instrumentation-fetch/README.md Outdated Show resolved Hide resolved
@pahiz @trentm
Update experimental/CHANGELOG.md
015b1df 
Co-authored-by: Trent Mick <trentm@gmail.com>
@JamieDanielson
Copy link
Member

JamieDanielson commented Jun 3, 2024 •
edited
Loading

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 •
edited
Loading

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

@JamieDanielson
Apply suggestions from code review
5da1639 
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.

@JamieDanielson JamieDanielson requested a review from trentm June 7, 2024 15:56
JamieDanielson
JamieDanielson approved these changes Jun 7, 2024
View reviewed changes
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?

@hashim21223445 hashim21223445 mentioned this pull request Jul 28, 2024
Open
@krischarbonneau krischarbonneau mentioned this pull request Jul 30, 2024
Merged
This was referenced Aug 9, 2024
Open
Open
This was referenced Sep 12, 2024
Open
Open
Open
Open
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
@pahiz @trentm
@JamieDanielson @Zirak
refactor(instr-fetch): move fetch to use SEMATRR (open-telemetry#4632)
6c2cf1c 
* refactor(instr-fetch): move fetch to use SEMATRR

* Update experimental/CHANGELOG.md

Co-authored-by: Trent Mick <trentm@gmail.com>

* Apply suggestions from code review

update readme table to use attribute strings

* move changelog entry to unreleased

---------

Co-authored-by: Trent Mick <trentm@gmail.com>
Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>
This was referenced Sep 16, 2024
Open
Open
@saidsef saidsef mentioned this pull request Sep 18, 2024
Merged
@micahg micahg mentioned this pull request Sep 18, 2024
Closed
@danqueb danqueb mentioned this pull request Sep 19, 2024
Open
This was referenced Sep 19, 2024
Open
Open
@zx8086 zx8086 mentioned this pull request Sep 19, 2024
Merged
This was referenced Sep 20, 2024
Open
Open
This was referenced Sep 24, 2024
Open
Open
This was referenced Sep 24, 2024
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Oct 3, 2024
Open
Open
@rreckless rreckless mentioned this pull request Oct 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pichlermarc pichlermarc pichlermarc left review comments

@JamieDanielson JamieDanielson JamieDanielson approved these changes

@trentm trentm trentm 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.
5 participants
@pahiz @david-luna @JamieDanielson @trentm @pichlermarc