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

add examples for async counter, updown counter, gauge #2144

Merged
merged 19 commits into from
Jan 23, 2023

Conversation

Omkar-Waingankar
Copy link
Contributor

@Omkar-Waingankar Omkar-Waingankar commented Jan 6, 2023

Notes:

  • added descriptions and code examples for various async instruments
  • i would like to point out that the descriptions earlier in the document for synchronous counters, etc. slightly deviate from my description. in particular, i reference the terms additive and monotonic which are not necessarily referenced in earlier descriptions. i think we should agree on making all of these consistent before merging.
  • i relied on this page to inform my descriptions, and this is also how i interpreted the various metrics when implementing them in Golang: https://opentelemetry.io/docs/reference/specification/metrics/supplementary-guidelines/

Preview: https://deploy-preview-2144--opentelemetry.netlify.app/docs/instrumentation/js/instrumentation

@Omkar-Waingankar Omkar-Waingankar requested review from a team as code owners January 6, 2023 22:11
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 6, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Omkar-Waingankar / name: Omkar Waingankar (6c3e939)

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.

Looks good - thank you for working on this! 🙂
Just a few nits 🙂

content/en/docs/instrumentation/js/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/js/instrumentation.md Outdated Show resolved Hide resolved
@Omkar-Waingankar
Copy link
Contributor Author

@svrnm @pichlermarc just address your comments on this PR, and also added a section on views as per the issue #2052

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.

Minor copy-edits, looking pretty good!

content/en/docs/instrumentation/js/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/js/instrumentation.md Outdated Show resolved Hide resolved
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.

Some more copy-edits

content/en/docs/instrumentation/js/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/js/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/js/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/js/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/js/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/js/instrumentation.md Outdated Show resolved Hide resolved
Omkar-Waingankar and others added 7 commits January 11, 2023 09:53
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
@Omkar-Waingankar
Copy link
Contributor Author

@cartermp thanks for all the suggestions - just addressed the last two comments on this PR in my latest commit

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.

Thank you! I like the changes.

@open-telemetry/javascript-approvers could you take another look? In particular the Views section?

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.

Thanks again for putting in the time to work on this 🙂
The view section is looking great. I have left two optional comments that I'll leave up to you. 🙂

content/en/docs/instrumentation/js/instrumentation.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/js/instrumentation.md Outdated Show resolved Hide resolved
Omkar-Waingankar and others added 2 commits January 12, 2023 10:47
Co-authored-by: Marc Pichler <marcpi@edu.aau.at>
Co-authored-by: Marc Pichler <marcpi@edu.aau.at>
@Omkar-Waingankar
Copy link
Contributor Author

@pichlermarc thanks for the suggestions! i committed them as well - i think they really tie all the examples together for the views :)

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.

@svrnm et al., see inline for a question.

@Omkar-Waingankar, thanks for your PR! One nit: please line-wrap paragraphs to 80 chars.

Omkar-Waingankar and others added 5 commits January 12, 2023 11:45
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
@cartermp
Copy link
Contributor

@chalin any further suggestions?

@svrnm
Copy link
Member

svrnm commented Jan 23, 2023

will merge this now

@svrnm svrnm merged commit 1d0f10f into open-telemetry:main Jan 23, 2023
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