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

Update metrics docs for JS #3853

Merged
merged 9 commits into from
Feb 15, 2024
Merged

Conversation

svrnm
Copy link
Member

@svrnm svrnm commented Jan 24, 2024

@svrnm svrnm requested review from a team January 24, 2024 14:23
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
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
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

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
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)

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.

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!

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants