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

Avoid Activity.SetCustomPropery usage for Resource. #1397

Closed
cijothomas opened this issue Oct 26, 2020 · 1 comment · Fixed by #1463
Closed

Avoid Activity.SetCustomPropery usage for Resource. #1397

cijothomas opened this issue Oct 26, 2020 · 1 comment · Fixed by #1463
Assignees
Labels
bug Something isn't working
Milestone

Comments

@cijothomas
Copy link
Member

cijothomas commented Oct 26, 2020

Currently TracerProviderSdk stores Resource inside every Activity. https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry/Trace/TracerProviderSdk.cs#L81
Internally, resource is stored inside Activity.SetCustomProperty. Perf benchmark shows that the 1st time CustomProperty is used, we pay a big price, with subsequent ones having no impact. We already modified all instrumentations to not user CustomProperty. Now the only reason for using CustomProperty is to store Resource.

Proposing to avoid CustomProperty by moving Resource elsewhere. Options include:

  1. Store resource inside activity.TagObject (it now support string,obj, so feasible to do this). This ensures every span/activity will be associated with a resource, as required by the spec.

  2. Dont store Resource anywhere inside the Activity itself. Instead pass it to all the Processors. TracerProviderSdk knows about Resource and about Processors. So it can set the Resource to the Processors. We may introduce some new interface like ISetResource(resoiurce), and for those processors which implements this, have TracerProviderSdk call it and set the resource.
    This ensures processor/exporter has a way to get the resource associated, but without actually having to store it in every activity. This should be a good win for perf.

  3. Similar to 2, introduce an interface ISetProvider that can be used to pass provider (TracerProvider, MeterProvider, LogProvider, etc.) to processors, exporters, etc.

Happy to hear more ideas. This was based on discussions with @eddynaka

@cijothomas cijothomas added the bug Something isn't working label Oct 26, 2020
@cijothomas cijothomas added this to the 0.8.0-beta (Beta 5) milestone Oct 26, 2020
@cijothomas
Copy link
Member Author

My preference is to store Resource inside TagObject, just like Status. We can use the keyname as "otel.resource" (status uses otel.status_code). We need to provide general guidance that any tags with reserved prefix "otel.", should not be treated as regular tags.

@cijothomas cijothomas added the help wanted Good for taking. Extra help will be provided by maintainers label Oct 28, 2020
@CodeBlanch CodeBlanch self-assigned this Nov 3, 2020
@CodeBlanch CodeBlanch removed the help wanted Good for taking. Extra help will be provided by maintainers label Nov 3, 2020
kzu added a commit to devlooped/Merq that referenced this issue Jul 13, 2023
This optimization means we don't pay the extra allocation and cost unnecessarily unless someone is actually requesting all data from activities.

See open-telemetry/opentelemetry-dotnet#1397
kzu added a commit to devlooped/Merq that referenced this issue Jul 13, 2023
This optimization means we don't pay the extra allocation and cost unnecessarily unless someone is actually requesting all data from activities.

See open-telemetry/opentelemetry-dotnet#1397
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment