-
Notifications
You must be signed in to change notification settings - Fork 772
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
Resource allocation improvement #1463
Resource allocation improvement #1463
Conversation
…f a TracerProvider into lower levels (Processor, Exporter, etc.).
Thanks @CodeBlanch! This looks the best option so far. LGTM overall. Will mark approved when moved as non-draft. |
Codecov Report
@@ Coverage Diff @@
## master #1463 +/- ##
==========================================
+ Coverage 81.13% 81.19% +0.05%
==========================================
Files 232 233 +1
Lines 6309 6311 +2
==========================================
+ Hits 5119 5124 +5
+ Misses 1190 1187 -3
|
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporter.cs
Outdated
Show resolved
Hide resolved
Stepping back question - do we expect that .NET would add |
🤷 IMO Resource is set by the user controlling the application and then used by the Exporter(s) they select. API doesn't know about it at all. So there's not a lot of value for .NET to own that and expose something. But I'm not privy to the planning being done 😄 |
Since we are not confident of pursuing to make .NET support this natively, we need to do something to associate Resource with trace/metric/log. We can explore:
|
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.
LGTM.
@reyang Would you please take another look at this? Removed the |
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.
very nice!!
@@ -20,17 +20,17 @@ jobs: | |||
- name: Setup .NET Core 2.1 | |||
uses: actions/setup-dotnet@v1 | |||
with: | |||
dotnet-version: 2.1.807 | |||
dotnet-version: 2.1.811 |
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.
I was getting an error that 2.1.807 could not be found. Bumped to latest.
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.
we could change to 2.1.x and 3.1.x and remove the rsync script.
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.
I ended up getting a conflict on the .net5 PR so while I was in there merging I switched to the ".x" for 2.1 & 3.1 versions.
@@ -46,6 +46,13 @@ public sealed override void OnStart(T data) | |||
/// <inheritdoc /> | |||
public abstract override void OnEnd(T data); | |||
|
|||
internal override void SetParentProvider(BaseProvider parentProvider) |
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.
@cijothomas I needed a way to set the Exporter.ParentProvider when the Processor's gets set. Internal to SDK though.
LGTM. It reminded me of DOM APIs. |
Fixes #1397.
Changes
To avoid an allocation on every Activity (a result of storing Resource as a custom property) expose
ParentProvider
and aGetResource
extension to Exporters.Public API Changes
TODOs:
CHANGELOG.md
updated for non-trivial changes