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: resource-detector should not be tied to tracing #5188

Merged
merged 18 commits into from
Jan 18, 2024

Conversation

luizhlelis
Copy link
Contributor

Fixes #3027
Design discussion issue #3027

Changes

  1. Moving resource to its own section;
  2. Add links to it in the trace, metric, and log sections.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@luizhlelis luizhlelis requested a review from a team December 16, 2023 20:48
@cijothomas
Copy link
Member

Thank you for tackling this!
#5185 Is tweaking the wordings on Resource. Once that is merged, lets bring that as well to this PR before merging.

@utpilla
Copy link
Contributor

utpilla commented Dec 18, 2023

@luizhlelis #5185 is merged. Please resolve the merge conflicts.

Follow [this](#resource-detector) document
to learn about writing custom resource detectors.

The snippet below shows configuring the `Resource` associated with the provider.
Copy link
Member

Choose a reason for hiding this comment

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

this section is tricky, as it is tied to Tracing signal.. We need to document all three with SDK.CreateProvider and the OTel.Ext.Hosting style of configuration of resource.

The env variables part is good for all three.

Copy link
Member

Choose a reason for hiding this comment

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

@luizhlelis See if you can fix this part too. If not, we can still merge, and do some follow ups later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @cijothomas, I've just removed that section from the resource doc. I left those definitions tied to the signals, for instance, trace, metrics, and logs. Please, let me know if that's what you want or if I should move them all to the new resource doc.

[OpenTelemetry](../../src/OpenTelemetry/README.md) package), and implement
the `Detect` method.

A demo ResourceDetector is shown [here](../trace/extending-the-sdk/MyResourceDetector.cs).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to simply show the code snippet to create a custom ResourceDetector here instead of pointing to docs/trace/...

You could then add code snippets that show how to add this custom ResourceDetector to all the three signals using both Sdk.Create/LoggerFactory.Create and Extensions.Hosting approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know if you would like to address these in this PR itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

codecov bot commented Dec 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6250307) 83.38% compared to head (733f01c) 83.13%.
Report is 14 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5188      +/-   ##
==========================================
- Coverage   83.38%   83.13%   -0.26%     
==========================================
  Files         297      271      -26     
  Lines       12531    11970     -561     
==========================================
- Hits        10449     9951     -498     
+ Misses       2082     2019      -63     
Flag Coverage Δ
unittests ?
unittests-Solution-Experimental 82.85% <100.00%> (?)
unittests-Solution-Stable 83.07% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...tation.AspNetCore/Implementation/HttpInListener.cs 89.72% <100.00%> (+0.14%) ⬆️

... and 32 files with indirect coverage changes

Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Dec 31, 2023
Copy link
Contributor Author

@luizhlelis luizhlelis left a comment

Choose a reason for hiding this comment

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

@cijothomas and @utpilla, when you have the chance, please review the PR again. Let me know if I need to do something else.

Follow [this](#resource-detector) document
to learn about writing custom resource detectors.

The snippet below shows configuring the `Resource` associated with the provider.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @cijothomas, I've just removed that section from the resource doc. I left those definitions tied to the signals, for instance, trace, metrics, and logs. Please, let me know if that's what you want or if I should move them all to the new resource doc.

[OpenTelemetry](../../src/OpenTelemetry/README.md) package), and implement
the `Detect` method.

A demo ResourceDetector is shown [here](../trace/extending-the-sdk/MyResourceDetector.cs).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jan 3, 2024
@cijothomas
Copy link
Member

Utkarsh and myself both were on vacation. I just came back today - will review by tomorrow. Thanks for waiting patiently!

@@ -0,0 +1,109 @@
# Resources

Copy link
Member

Choose a reason for hiding this comment

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

it would be good to give a background on what is resource etc. here in a follow up.

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

This does make resource less tied to tracing, so approving to merge.
We need to have a follow up to move the Resource readme from individual docs to the newly created common place. Specifically, content from https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/docs/metrics/customizing-the-sdk#resource (and logs, traces too), should be moved to the common doc as much as possible

@alanwest alanwest merged commit 89e5382 into open-telemetry:main Jan 18, 2024
30 checks passed
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.

[Docs] ResourceDetector should not be tied to tracing
5 participants