Store resource in tracer SDK #352
Conversation
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
pavolloffay
requested review from
bogdandrutu,
carlosalberto,
SergeyKanzhelev,
songy23,
tylerbenson and
yurishkuro
as code owners
Codecov Report
@@ Coverage Diff @@
## master #352 +/- ##
============================================
- Coverage 63.38% 63.33% -0.06%
Complexity 320 320
============================================
Files 56 56
Lines 1251 1252 +1
Branches 112 112
============================================
Hits 793 793
- Misses 419 420 +1
Partials 39 39
Continue to review full report at Codecov.
|
| public void setResource(Resource resource) {} | ||
| public void setResource(Resource resource) { | ||
| this.resource = resource; | ||
| } | ||
|
|
||
| @Override | ||
| public Resource getResource() { |
There was a problem hiding this comment.
is there a comment on API to not cache this value? We need to clearly state that the object returned by this API can be replaced in runtime at any moment
There was a problem hiding this comment.
Some questions:
- I am not sure why do we need a get on this?
- Also not sure we need a set, I think we can just offer the ability to set the initial resource during initialization time of the TracerSdk.
There was a problem hiding this comment.
I did this PR to move forward with the impl. Setter and getter on the resource seem not required in my opinion. I thought that this has been discussed as it is in the API.
I think that setting a resource belongs to initialization. Getter seems also redundant.
There was a problem hiding this comment.
@pavolloffay I am sorry that I kind of moved the discussion a bit here and that it happen in your PR. You did a good job on this PR, but want to make sure we do the best of the project.
I think we are on the same opinion about what is needed here.
There was a problem hiding this comment.
So do we remove these methods completely fro the tracer API?
There was a problem hiding this comment.
That's what I am proposing, waiting for @SergeyKanzhelev and @carlosalberto to see what do they think?
There was a problem hiding this comment.
Oh, I remember we talking about this some time ago ;) Yeah, it makes sense to have this as part of the Tracer initialization only - leaving the get() part would be fine, but probably we don't really need to expose it in the main API.
There was a problem hiding this comment.
One reason to expose it is to allow to inject this resource information in outgoing requests and response headers. There are systems that exchange this information.
There was a problem hiding this comment.
Adding an API is backwards compatible, until we have a clear case can we remove the API for the moment and add it later if needed.
There was a problem hiding this comment.
Yeah, I'd propose go with the 'safe' route now and remove it for the moment, and add it if/when actually needed.
|
@SergeyKanzhelev are you approving to remove these or the current PR? |
|
oh, I didn't realize get for resource already was there. I glanced over and though that getter was removed so we only have |
|
So, we agreed to remove the getter from Tracer. What about set, do we need an explicit set or we want only the SDK to allow that (not the API)? I see no reason for an instrumentation plugin to set the resources globally, at most we need what we discussed in the components issue, to add more data about the instrumented component and merge these into the default tracer Resource. |
So I feel like we should set this only a ctor time (at least for the time being). |
If Resoruce information is obtained from metadata REST call in AWS and Azure, would you require for the process to hold on on doing anything before that call will complete? Setter would allow to update Resource information later in the process. |
|
@SergeyKanzhelev I think this is very specific to every implementation. I agree with you that a proper implementation should not necessary hold the start of the process for this. I think SDK can have a way to set this later. |
|
@SergeyKanzhelev also that is one of the reason we want to default read the Resource from env variables to ensure we don't block the process to much. |
|
so resource auto-discovery modules would depend on SDK? I'm fine with it, just want to make sure it's understood. Will there be any mention of Resource on API than? Only for |
|
I think we want people to add more labels, as discussed in open-telemetry/opentelemetry-specification#10. These "resources" will be merged with the auto-detected by the SDK. |
|
@bogdandrutu than it probably should be |
|
done in #387 |
Updates #281
Signed-off-by: Pavol Loffay ploffay@redhat.com