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.

Sign up for GitHub

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

Store resource in tracer SDK #352

Closed
wants to merge 1 commit into from
Closed

Store resource in tracer SDK #352

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions sdk/src/main/java/io/opentelemetry/sdk/trace/TracerSdk.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ public class TracerSdk implements Tracer {
// operations are visible on other CPUs as well.
private volatile TraceConfig activeTraceConfig = TraceConfig.DEFAULT;

private volatile Resource resource;

@Override
public Span getCurrentSpan() {
return ContextUtils.getValue();
Expand All @@ -51,11 +53,13 @@ public Span.Builder spanBuilder(String spanName) {
}

@Override
public void setResource(Resource resource) {}
public void setResource(Resource resource) {
this.resource = resource;
}

@Override
public Resource getResource() {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Some questions:

  1. I am not sure why do we need a get on this?
  2. 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

So do we remove these methods completely fro the tracer API?

Copy link
Member

Choose a reason for hiding this comment

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

That's what I am proposing, waiting for @SergeyKanzhelev and @carlosalberto to see what do they think?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'd propose go with the 'safe' route now and remove it for the moment, and add it if/when actually needed.

return null;
return resource;
}

@Override
Expand Down