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

docs: add documentation about using Metrics API/SDK #3349

Closed
wants to merge 18 commits into from

Conversation

weyert
Copy link
Contributor

@weyert weyert commented Oct 20, 2022

Which problem is this PR solving?

Adds documentation about using the Metrics API/SDK to allow the release of Metrics GA

Fixes # (issue)

Short description of the changes

Added a metrics.md documentation file

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Read the newly added documentation

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 20, 2022

CLA Missing ID CLA Not Signed

@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Merging #3349 (1f14a8c) into main (4420402) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3349   +/-   ##
=======================================
  Coverage   93.48%   93.48%           
=======================================
  Files         243      243           
  Lines        7332     7332           
  Branches     1512     1512           
=======================================
  Hits         6854     6854           
  Misses        478      478           

@legendecas
Copy link
Member

We talked about documentation maintenance at #3255. End-user (primarily API-packages related) documentation can be moved to the website instead.

SDK documentation can contain more language-specific design and need to be maintained in each language repo.

@dyladan
Copy link
Member

dyladan commented Oct 20, 2022

ping @cartermp @svrnm

@dyladan
Copy link
Member

dyladan commented Oct 20, 2022

We talked about documentation maintenance at #3255. End-user (primarily API-packages related) documentation can be moved to the website instead.

SDK documentation can contain more language-specific design and need to be maintained in each language repo.

I think we can iterate on this draft PR where the JS approvers see it most easily and open a website PR when we're happy?

@svrnm
Copy link
Member

svrnm commented Oct 20, 2022

We talked about documentation maintenance at #3255. End-user (primarily API-packages related) documentation can be moved to the website instead.
SDK documentation can contain more language-specific design and need to be maintained in each language repo.

I think we can iterate on this draft PR where the JS approvers see it most easily and open a website PR when we're happy?

We build a preview of the website with the changes over at open-telemetry/opentelemetry.io, that's really helpful and might be a reason to have this draft over there

@cartermp
Copy link
Contributor

Yep, agreed with @svrnm

If there's some rapid feedback for the code samples that moves faster here, then I think that's fine to get those ironed out ASAP.

Otherwise, since we build previews of the site on all PRs, it's very fast to iterate on how it looks overall, ensuring links across the site work, ensuring it "flows" with the rest of the docs well, etc.

@weyert
Copy link
Contributor Author

weyert commented Oct 21, 2022

Sorry, what's the plan now? I am a bit confused

@weyert
Copy link
Contributor Author

weyert commented Oct 21, 2022

I have pushed an update to the metrics.md to have a roughly structure of bits I want to write

@cartermp
Copy link
Contributor

cartermp commented Oct 22, 2022

@weyert see here: #3026 (comment)

I think in the interest of getting something done quickly, I won't block any effort to get what you've written as-is into this repository. If it's faster for you to get some good code samples written and in a decent shape, we can take it and mold it into the shape we need for the docs on the website.

If there are developer docs (i.e., for people extending metrics with the SDK) then those can live in this repository.

However, to document metrics fully for end-users, it will have to be on the website repo and following the outlined structure linked to from the comment above.

@weyert
Copy link
Contributor Author

weyert commented Oct 22, 2022

Cool, I will finish the outline and then we can use it as a first iteration so we can unblock,the release! Sounds good?

@cartermp
Copy link
Contributor

Yep, I dig it!

@weyert weyert marked this pull request as ready for review October 24, 2022 13:27
@weyert weyert requested a review from a team October 24, 2022 13:27
@weyert
Copy link
Contributor Author

weyert commented Oct 24, 2022

I have finished my first iteration for the Metrics documentation. I think it's a good start for using metrics. In my opinion it discusses the most import concepts

cc @cartermp


_Metrics API Reference: <https://open-telemetry.github.io/opentelemetry-js-api/classes/metricseapi.html>_

- [Acquiring a Meter](#acquiring-a-meter)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add a "quick start" at the top which shows the minimum to set up the API and SDK and export to something (prom or console i guess). Right now I miss a full e2e example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, having a "right way" to set up the SDK would be ideal. I can take that + what's here and more or less plant it in the website.

Re: a full e2e quick start ... perhaps, if there's a way to include that here and base it off of the python quickstart that could be cool. I plan on eventually reworking the Node quickstart in the docs to follow this pattern. So long as I know the best way to initialize the SDK/API for metrics, I can just take that on though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will work on a getting started based on the Python one as first chapter of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added something @cartermp @dyladan

doc/metrics.md Outdated Show resolved Hide resolved
doc/metrics.md Outdated Show resolved Hide resolved
doc/metrics.md Outdated Show resolved Hide resolved
doc/metrics.md Outdated Show resolved Hide resolved
@weyert
Copy link
Contributor Author

weyert commented Oct 25, 2022

Let me know, if more content needs to be written. @dyladan @cartermp
If so, I will try to schedule some time in tomorrow to work on writing

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.

Hi @weyert 🙂
Thanks for working on the docs, this is a great contribution. 🎉

I added some nits and recommendations, feel free to ignore if something does not make sense (I'm not a native english speaker, so there might be some that don't make sense 😅) 🙂

doc/metrics.md Outdated Show resolved Hide resolved
doc/metrics.md Outdated Show resolved Hide resolved
doc/metrics.md Outdated Show resolved Hide resolved
doc/metrics.md Outdated Show resolved Hide resolved
doc/metrics.md Outdated Show resolved Hide resolved
doc/metrics.md Outdated Show resolved Hide resolved
doc/metrics.md Outdated Show resolved Hide resolved
doc/metrics.md Outdated Show resolved Hide resolved
doc/metrics.md Outdated Show resolved Hide resolved
doc/metrics.md Outdated Show resolved Hide resolved
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.

@weyert Thanks for incorporating the feedback 🙂
Looks like the e-mail is missing on your commits, so EasyCLA cannot associate your commits with your account. 🤔
You might have to rewrite your history and add you e-mail your commits to get it working again. 🙂

Once this and the linting errors are resolved, this should be good to go IMO 🙂

@cartermp
Copy link
Contributor

@weyert looks good to me. This should be more than enough for us to get some docs on the website going.

@weyert weyert closed this Oct 25, 2022
@weyert
Copy link
Contributor Author

weyert commented Oct 25, 2022

@cartermp @pichlermarc find the new PR with correct commit author here:
#3360

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.

6 participants