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

Reorganize demo feature documentation #3714

Merged
merged 5 commits into from
Jan 3, 2024

Conversation

flands
Copy link
Contributor

@flands flands commented Dec 28, 2023

  • Create a nested section called features
  • Organize feature index page
  • Add feature flags under features subsection
  • Add signal coverage under features subsection

@flands flands requested review from a team December 28, 2023 13:24
@flands flands force-pushed the flands/features branch 3 times, most recently from 4fdbcc2 to 13f501a Compare December 28, 2023 13:40
- Create a nested section called features
- Organize feature index page
- Add feature flags under features subsection
- Add signal coverage under features subsection
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.

Content looks good overall. Could you run npm run fix to address formatting and stuff? Thanks!

@flands flands requested a review from a team December 29, 2023 21:20
@flands
Copy link
Contributor Author

flands commented Dec 29, 2023

OK, I think I fixed the links - at least they appear to work with npm run serve:netlify but they don't work with npm run serve as hugo aliases appear to be disabled... Is there a different way to do this?

@cartermp
Copy link
Contributor

Hmm, seems I still get these errors with npm run check:links:

blog/2023/demo-birthday/index.html
  target does not exist --- blog/2023/demo-birthday/index.html --> /docs/demo/feature-flags/
blog/2022/announcing-opentelemetry-demo-release/index.html
  target does not exist --- blog/2022/announcing-opentelemetry-demo-release/index.html --> /docs/demo/trace-features/
  target does not exist --- blog/2022/announcing-opentelemetry-demo-release/index.html --> /docs/demo/metric-features/
docs/demo/architecture/index.html
  target does not exist --- docs/demo/architecture/index.html --> /docs/demo/metric-features/
  target does not exist --- docs/demo/architecture/index.html --> /docs/demo/trace-features/
docs/demo/index.html
  target does not exist --- docs/demo/index.html --> feature-flags
  target does not exist --- docs/demo/index.html --> feature-flags/
  target does not exist --- docs/demo/index.html --> metric-features/
  target does not exist --- docs/demo/index.html --> trace-features/

@flands
Copy link
Contributor Author

flands commented Dec 30, 2023

Yes, me too, and this appears to be because hugo.yaml has disableAliases: true # We do redirects via Netlify's _redirects file. If you want it to work, you have to run npm run serve:netlify instead of npm run serve. I'm not sure how to proceed here...

@cartermp
Copy link
Contributor

Ergh, failed twice to push changes to your branch due to git permissions -- here's a patch file of the changes you need to make this work.
0001-fix-links.patch

@puckpuck
Copy link
Contributor

puckpuck commented Jan 2, 2024

I like some of where this is going, but I see it adding even more confusion.

The new section includes Telemetry coverage and Feature Flags. I don't believe they shouldn't be grouped. Should the new section be called "Telemetry Features" instead, and Feature Flags be moved back up? Also, I would like to see "Manual Span Attributes" pulled into this section.

Thinking into the future, we currently have a gap with manual metrics needing to be documented, and this section would be a great spot to add them.

@flands
Copy link
Contributor Author

flands commented Jan 2, 2024

Ergh, failed twice to push changes to your branch due to git permissions -- here's a patch file of the changes you need to make this work. 0001-fix-links.patch

Thanks and yeah, I feared this was the answer - I'll file a bug about aliases not working properly. I'm not sure fixing all links is scalable long-term.

@cartermp
Copy link
Contributor

cartermp commented Jan 3, 2024

@puckpuck any further thoughts on this PR?

Copy link
Contributor

@puckpuck puckpuck 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. :shipit:

@cartermp cartermp merged commit 735babe into open-telemetry:main Jan 3, 2024
14 checks passed
@flands flands deleted the flands/features branch January 3, 2024 12:14
@@ -1,6 +1,6 @@
---
title: Feature Flags
aliases: [feature_flags]
aliases: [feature_flags, ../feature_flags/]
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the intent here? This added alias effectively pushes a page alias outside of the Demo section, which we don't want IMHO.

Copy link
Contributor

Choose a reason for hiding this comment

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

@chalin
Copy link
Contributor

chalin commented Jan 18, 2024

Btw, thanks for the PR @flands, this slight reorg is an improvement IMHO! 👍🏻

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.

4 participants