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

Update metrics docs for JS #3853

Merged
merged 9 commits into from
Feb 15, 2024
Merged

Update metrics docs for JS #3853

merged 9 commits into from
Feb 15, 2024

Conversation

svrnm
Copy link
Member

@svrnm svrnm commented Jan 24, 2024 •
edited by chalin
Loading

@svrnm
Update metrics docs for JS
47e0693 
Signed-off-by: svrnm <neumanns@cisco.com>
@svrnm
Update metrics in JS
41191c6 
Signed-off-by: svrnm <neumanns@cisco.com>
@svrnm svrnm requested review from a team January 24, 2024 14:23
theletterf
theletterf reviewed Jan 24, 2024
View reviewed changes
Copy link
Member

@theletterf theletterf left a comment

Choose a reason for hiding this comment

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

Thanks! Just some style comments and nits.

content/en/docs/concepts/signals/metrics.md Outdated Show resolved Hide resolved
content/en/docs/concepts/signals/metrics.md Outdated Show resolved Hide resolved
content/en/docs/concepts/signals/metrics.md Outdated Show resolved Hide resolved
content/en/docs/concepts/signals/metrics.md Outdated Show resolved Hide resolved
content/en/docs/concepts/signals/metrics.md Outdated Show resolved Hide resolved
content/en/docs/languages/js/instrumentation.md Outdated Show resolved Hide resolved
@svrnm @theletterf
Apply suggestions from code review
3047c41 
Co-authored-by: Fabrizio Ferri-Benedetti <fferribenedetti@splunk.com>
cartermp
cartermp reviewed Jan 27, 2024
View reviewed changes
Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Overall looks fine. @open-telemetry/javascript-approvers PTAL

chalin
chalin reviewed Jan 30, 2024
View reviewed changes
content/en/docs/languages/js/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/js/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/js/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/js/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/js/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/languages/js/instrumentation.md Outdated Show resolved Hide resolved
@svrnm svrnm mentioned this pull request Feb 1, 2024
Open
@svrnm @chalin
Apply suggestions from code review
d1f2445 
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
svrnm
svrnm commented Feb 1, 2024
View reviewed changes
content/en/docs/concepts/signals/metrics.md
Copy link
Member Author

Choose a reason for hiding this comment

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

@theletterf @chalin @cartermp just to add some context: the changes to this file are coming from some text I moved out from the JS docs to the general docs, I may not have all the knowledge about all the text if & how it makes sense, so if you have any suggestions to make that text better, provide me with inline suggestions:-)

@svrnm
Copy link
Member Author

svrnm commented Feb 2, 2024

@open-telemetry/docs-approvers , @open-telemetry/javascript-approvers please take another look. I removed a lot of the conceptual content from the "instrumentation" page since this belongs to the concepts page (and there I point to the guideline in the specs)

chalin
chalin approved these changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Overall LGTM. I'll let the others do a more thorough review.

cartermp
cartermp approved these changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Similarly a ✅ from me, including after the changes since I last reviewed

@cartermp cartermp added the sig-approval-missing Co-owning SIG didn't provide an approval label Feb 3, 2024
@svrnm
Copy link
Member Author

svrnm commented Feb 14, 2024

@open-telemetry/javascript-approvers PTAL!

pichlermarc
pichlermarc approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

This looks good, thanks for working on this @svrnm

content/en/docs/concepts/signals/metrics.md Outdated Show resolved Hide resolved
@svrnm svrnm removed the sig-approval-missing Co-owning SIG didn't provide an approval label Feb 15, 2024
@svrnm
Copy link
Member Author

svrnm commented Feb 15, 2024

This looks good, thanks for working on this @svrnm

thanks for taking a look!

@svrnm svrnm merged commit e73b510 into open-telemetry:main Feb 15, 2024
14 checks passed
@svrnm svrnm deleted the update-metrics-in-js branch February 15, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theletterf theletterf theletterf left review comments

@chalin chalin chalin approved these changes

@cartermp cartermp cartermp approved these changes

@pichlermarc pichlermarc pichlermarc approved these changes

Assignees
No one assigned
Labels
sig:javascript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@svrnm @theletterf @chalin @cartermp @pichlermarc