-
Notifications
You must be signed in to change notification settings - Fork 765
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
Improve docs on Resources to encourage its usage #5185
Improve docs on Resources to encourage its usage #5185
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #5185 +/- ##
==========================================
+ Coverage 83.45% 83.50% +0.04%
==========================================
Files 297 297
Lines 12397 12397
==========================================
+ Hits 10346 10352 +6
+ Misses 2051 2045 -6
Flags with carried forward coverage won't be shown. Click here to find out more. |
…mas/opentelemetry-dotnet into cijothomas/resource-docs
.ConfigureResource(res => res.AddService("example-service")) | ||
.ConfigureResource(r => r.AddAttributes(new List<KeyValuePair<string, object>> | ||
{ | ||
new KeyValuePair<string, object>("static-attribute1", "v1"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably better to use a meaningful key instead of generic names. Something like MachineId, ProcessId, Region etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, will steal something from opentelemetry.io example in a subsequent PR.
is built, by calling the `Build()` method on the `MeterProviderBuilder`. | ||
The `ConfigureResource` method on `MeterProviderBuilder` can be used to | ||
configure the resource on the provider. `ConfigureResource` accepts an `Action` | ||
to configure the `ResourceBuilder`. Multiple calls to `ConfigureResource` can be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple calls to
ConfigureResource
can be made.
Rephrasing suggestion: "It's safe to call ConfigureResource
multiple times."
attributes from various sources. For example, `AddService()` adds | ||
[Service](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/resource/README.md#service) | ||
resource. `AddAttributes` can be used to add any additional attribute to the | ||
`Resource`. It also allows adding `ResourceDetector`s. |
There was a problem hiding this comment.
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 add a link for the term ResourceDectectors to this doc: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md#detecting-resource-information-from-the-environment
the process as Resources, instead of adding them as attributes(tags) on each | ||
measurement. | ||
|
||
Follow [this](../../trace/extending-the-sdk/README.md#resource-detector) document |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh you already have a link to ResourceDetector information here. This should be moved above right after the text: "It also allows adding ResourceDetector
s."
#5188 has been submitted since this was opened. We need to merge this in, then do 5188. Few suggestions in this PR need to be ported to 5188 as well. |
Improving the wording on Resources, modifying example to show adding custom attributes are the main changes. Also refactoring the best practices section to better point to Resource section.
Note: This repo interchangeably uses the word Tag vs Attributes. Need a follow up to ensure consistent usage throughout. Its not entirely feasible as Resource picked the "Attribute" terminology, but Traces/Metrics picked "Tags" terminology :(