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

migrate to semconv for otelmux and implementation the package in metrics #6648

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

migrate to semconv for otelmux and implementation the package in metrics #6648

wants to merge 1 commit into from

Conversation

martinyonatann
Copy link

Address issue #6638

@martinyonatann martinyonatann requested a review from a team as a code owner January 19, 2025 18:39
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Jan 19, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: martinyonatann / name: Martin Yonatan Pasaribu (379f764)

@github-actions github-actions bot requested a review from akats7 January 19, 2025 18:39
@dmathieu
Copy link
Member

Thank you for this! ❤️

This PR does a lot more than just setup semconv.
In order to allow for better reviews, could you split it into multiple PRs?
From a first look, I'd do at least four:

  • Setup the body wrapper and use it.
  • Change the default span name behavior.
  • Introduce the semconv package, and use it with tracing only.
  • Introduce metrics support.

Striving for atomic PRs (each PR changes a single thing) makes for much speeder reviews.

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 20, 2025
View reviewed changes
instrumentation/github.com/gorilla/mux/otelmux/request/resp_writer_wrapper.go Outdated
w.writeHeader(http.StatusOK)
}

n, err := w.ResponseWriter.Write(p)

Check warning

Code scanning / CodeQL

Reflected cross-site scripting

Cross-site scripting vulnerability due to [user-provided value](1).
@martinyonatann
Copy link
Author

martinyonatann commented Jan 20, 2025 •
edited
Loading

thankyou for the feedback @dmathieu ! ❤️
You're absolutely right, this PR ended up covering more ground than initially intended. i appreciate your suggestion to split it into atomic PRs for easier review. i'll get started on breaking it down into the following :

This approach should make the review process smother and more efficient. i'll prioritize reorganizing these changes and will submit the individual PRs soon. Let me know if there are any additional points you'd like me to address.

@dmathieu dmathieu marked this pull request as draft January 20, 2025 09:27
This was referenced Jan 20, 2025
Open
Open
@dmathieu dmathieu mentioned this pull request Jan 24, 2025
Draft
@martinyonatann martinyonatann force-pushed the feat/migrate-to-semconv-for-otelmux branch from 50b35e9 to 379f764 Compare January 27, 2025 06:34
@martinyonatann martinyonatann changed the title migrate to semconv for otelmux #6638 migrate to semconv for otelmux and implementation the package in metrics Jan 27, 2025
@martinyonatann martinyonatann mentioned this pull request Jan 27, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmathieu dmathieu dmathieu left review comments

@akats7 akats7 Awaiting requested review from akats7

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.
2 participants
@martinyonatann @dmathieu