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 Metrics documentation to project #3360

Merged
merged 23 commits into from
Nov 10, 2022

Conversation

weyert
Copy link
Contributor

@weyert weyert commented Oct 25, 2022

Which problem is this PR solving?

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Short description of the changes

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?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A

Checklist:

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

@weyert weyert marked this pull request as ready for review October 25, 2022 14:56
@weyert weyert requested a review from a team October 25, 2022 14:56
@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

Merging #3360 (8047524) into main (7972edf) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 8047524 differs from pull request most recent head f227c73. Consider uploading reports for the commit f227c73 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3360      +/-   ##
==========================================
- Coverage   93.25%   93.24%   -0.02%     
==========================================
  Files         247      247              
  Lines        7344     7344              
  Branches     1511     1511              
==========================================
- Hits         6849     6848       -1     
- Misses        495      496       +1     
Impacted Files Coverage Δ
...-trace-base/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️

Copy link
Contributor Author

@weyert weyert left a comment

Choose a reason for hiding this comment

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

style: fix markdown double blank line

Copy link
Contributor Author

@weyert weyert left a comment

Choose a reason for hiding this comment

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

style: fix markdown linting issue

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 25, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the work!

CHANGELOG.md Outdated Show resolved Hide resolved
doc/metrics.md Outdated Show resolved Hide resolved
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
@dyladan
Copy link
Member

dyladan commented Nov 1, 2022

@weyert this looks good to me but can you open the PR on the website repo? I think the overall project is trying to move documentation to the website when reasonable

@dyladan
Copy link
Member

dyladan commented Nov 1, 2022

@cartermp may have an opinion there. Last I heard they were using submodules for some languages too. What I don't want is a situation where some docs are here and some are in the website repo.

@weyert
Copy link
Contributor Author

weyert commented Nov 1, 2022

@cartermp may have an opinion there. Last I heard they were using submodules for some languages too. What I don't want is a situation where some docs are here and some are in the website repo.

@cartermp Let me know, what you prefer :)

@cartermp
Copy link
Contributor

cartermp commented Nov 1, 2022

These should be on the website long-term. My comment on the past PR was that if it's easier to get initial feedback here, then that's good, but all end-user documentation should live on the website. So if this looks like it's good enough for folks, then I would recommend we either:

  • Merge here --> someone opens a PR on the docs site (I am happy to do so) --> rework to the shape outline here: Document latest Metrics SDK #3026 (comment) either immediately or after it's merged on the site
  • Everyone says "LGTM" and we open a PR on the docs site without merging here

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.

LGTM % some nits. Thanks for working on this. 🙂
Sorry for taking so long with the review, was out of office for a while.

CHANGELOG.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
CHANGELOG.md Outdated Show resolved Hide resolved
pichlermarc and others added 5 commits November 2, 2022 10:41
Co-authored-by: Marc Pichler <marcpi@edu.aau.at>
Co-authored-by: Marc Pichler <marcpi@edu.aau.at>
Co-authored-by: Marc Pichler <marcpi@edu.aau.at>
Copy link
Contributor Author

@weyert weyert left a comment

Choose a reason for hiding this comment

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

I have added the suggested changes

package.json Outdated Show resolved Hide resolved
@dyladan
Copy link
Member

dyladan commented Nov 8, 2022

Updated the PR to use the api package instead of api-metrics. Note that this will not be accurate until after the API is released so this should not be released before that.

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 🙂

@cartermp
Copy link
Contributor

cartermp commented Nov 9, 2022

So it sounds like the motion will be:

  • Merge here
  • We add metrics docs to the website
  • ....something something this file in this repo happens?

I've no strong opinion about what happens in this repo, but we'll want the docs in the website and in the outlined structure at some point.

@legendecas
Copy link
Member

legendecas commented Nov 9, 2022

@cartermp I'm not sure how other languages manage their documentation with the website project. Is there any procedure we can adopt in the JS repo too? I'd be happy to help to alleviate the burden of the website maintainers if this documentation can be moved to the website repo.

@cartermp
Copy link
Contributor

cartermp commented Nov 9, 2022

Perhaps I confused things with this comment by not advocating to move the PR to the website repo in the first place. Since Metrics docs don't exist yet, if it's faster to get feedback here then a PR here is probably best, but the end state would be to live in the docs. That was my understanding based on conversation in PRs.

All end-user non-api-reference JS docs live in the docs/website repo today, as is also the case for all other languages now aside from Go (which we're proposing to move soon too). These docs will also need to be changed to meet the outline laid out here (linked to from the metrics docs issue on this repo). To be honest, I'm not sure why there's been confusion here.

@dyladan
Copy link
Member

dyladan commented Nov 9, 2022

Perhaps I confused things with this comment by not advocating to move the PR to the website repo in the first place. Since Metrics docs don't exist yet, if it's faster to get feedback here then a PR here is probably best, but the end state would be to live in the docs. That was my understanding based on conversation in PRs.

I agree end state should be docs on website. PR was open here to get feedback.

All end-user non-api-reference JS docs live in the docs/website repo today, as is also the case for all other languages now aside from Go (which we're proposing to move soon too).

We can begin that process now as I believe everyone is happy with the state of these docs. I'll probably merge this so that there is something while the website docs are worked on but will remove these in another PR when that is done. Does that seem reasonable?

These docs will also need to be changed to meet the outline laid out here (linked to from the metrics docs issue on this repo).

Good to know. Thanks for the link.

@cartermp
Copy link
Contributor

cartermp commented Nov 9, 2022

Yep, makes sense! I think that's a good process

@dyladan
Copy link
Member

dyladan commented Nov 10, 2022

Lint expected to fail markdown link check until the sdk metrics docs are published. The link is correct

@dyladan dyladan merged commit 74e39c7 into open-telemetry:main Nov 10, 2022
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