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

Make it impossible to remove mandatory resource attributes that the SDK has defaults for #5595

Closed
jonatan-ivanov opened this issue Jul 3, 2023 · 12 comments
Labels
Bug Something isn't working

Comments

@jonatan-ivanov
Copy link
Member

Describe the bug
OTel semantic conventions mandate certain resource attributes to present, see telemetry sdk semantic conventions. I'm not sure why it is possible to remove the mandatory defaults but looking at the docs/examples, it seems it is intentional. I think it should be impossible to remove these attributes.

Steps to reproduce
This will erase the mandatory defaults:

SdkTracerProviderBuilder builder = SdkTracerProvider.builder()
        .setResource(Resource.create(Attributes.of(...)));

Users need to run extra circles to add them back:

SdkTracerProviderBuilder builder = SdkTracerProvider.builder()			 
       .setResource(Resource.getDefault().merge(Resource.create(Attributes.of(...))));

What did you expect to see?
Mandatory resource attributes should be always there even if the users call setResource on SdkTracerProviderBuilder.

What did you see instead?
Mandatory resource attributes are missing.

What version and what artifacts are you using?
Artifacts: opentelemetry-sdk
Version: 1.27.0
How did you reference these artifacts? implementation 'io.opentelemetry:opentelemetry-sdk:1.27.0'

Environment
Compiler: liberica-20.0.1+10
OS: MacOS 13.4.1

Additional context
N/A

@jkwatson
Copy link
Contributor

jkwatson commented Jul 5, 2023

Disagree that this is a bug. If a user explicitly replaces the entire resource, they should 100% be allowed to do that.

@jonatan-ivanov
Copy link
Member Author

I think I disagree a little, I think users should not be easily (if at all) do things that is clearly against the specs, assuming the Telemetry SDK SemConv specs are applicable to the OTel Java SDK:

The OpenTelemetry SDK MUST set the telemetry.sdk.name attribute to opentelemetry. [...]

Here (as a user), it is pretty unexpected to me that I need to manually add back the mandatory attributes if I want to add mines. I don't think this is a good user experience, this should be possible without me (the user) knowing about those default attributes.

@jkwatson
Copy link
Contributor

I think I disagree a little, I think users should not be easily (if at all) do things that is clearly against the specs, assuming the Telemetry SDK SemConv specs are applicable to the OTel Java SDK:

The OpenTelemetry SDK MUST set the telemetry.sdk.name attribute to opentelemetry. [...]

Here (as a user), it is pretty unexpected to me that I need to manually add back the mandatory attributes if I want to add mines. I don't think this is a good user experience, this should be possible without me (the user) knowing about those default attributes.

I wouldn't say that the Resource class is "The OpenTelemetry SDK" in any sense. If you build an instance of the SDK without overriding the default resource, you get the default resource, as specified. If you, as a user, decide to override that default resource, then you are choosing to do that, and that should 100% be allowed.

@jonatan-ivanov
Copy link
Member Author

I think it is more about SdkTracerProviderBuilder than Resource which I think is part of the SDK. I think the biggest issue is not simple to provide my own attributes in addition to the default ones (I need to manually add back the mandatory attributes) and also it is extremely simple to "accidentally" remove them. I think this is not a good user experience in terms of resource attribute customization. I think "set" is ok (though I'm still questioning breaking the spec) but I think there should be an "add" method too where the users don't need to know about any other special knowledge (what are the attributes the user is wiping).

@jack-berg
Copy link
Member

We talked about this and there seemed to be agreement in extending the Sdk{Signal}ProviderBuilder classes to include a new addResource(Resource) method, in addition to the existing setResource(Resource) method. The semantics of addResource would be to append / merge the resource with the existing, and to include javadoc recommending using addResource(Resource) instead of setResource(Resource) because as you mention, it is a bit to simple to accidentally remove the default resource attributes.

Coincidentally, @parth1601 has opened PR #5619 which does just this, albeit with a different end goal in mind. Goal of #5619 is to make it easy to customize the resource of one signal's resource when using autoconfigure (which I don't recommend).

@jack-berg
Copy link
Member

I belief this is resolved. Now that #5619 is merged, its more challenging to remove the default resource attributes.

@drok
Copy link

drok commented Nov 28, 2023

So how do you remove the default, but non-mandatory resources?

I am an end user of the opentelemetry SDK. I got it to work but there is a lot of noise collected in my backend. like the telemetry.sdk.* attributes. I want them gone. I'm not interested in a righteous debate. I just need sample code. Let me deal with the consequences of my actions myself, I'm a grownup.

Could the documentation provide an example?

@trask
Copy link
Member

trask commented Nov 28, 2023

hi @drok!

did you see @jack-berg's #5595 (comment) above?

We talked about this and there seemed to be agreement in extending the Sdk{Signal}ProviderBuilder classes to include a new addResource(Resource) method, in addition to the existing setResource(Resource) method. The semantics of addResource would be to append / merge the resource with the existing, and to include javadoc recommending using addResource(Resource) instead of setResource(Resource) because as you mention, it is a bit to simple to accidentally remove the default resource attributes.

@drok
Copy link

drok commented Nov 28, 2023

did you see @jack-berg's #5595 (comment) above?

Hi @trask!

I certainly saw jack-berg's comment, and the merged branch that implements the addResource method, which is supposed to replace rather than append the resources. I recognize the solution I'm looking for is in that neighbourhood.

However, it remains that as an (new to opentelemetry, and telemetry) end user, I'm seeing unwanted strings sent to my backend, and I'm not yet able to figure out how to implement the setResource call in my javascript app. I saw the sample code in the pull request description, but it is Java and quite different from the setup example I've followed:

        let traceExporter = new OTLPTraceExporter({ url: 'http://192.168.20.152:4318/v1/traces' });

        this.sdk = new optl.NodeSDK({
            traceExporter,
            instrumentations: [],
            resourceDetectors: [],
            autoDetectResources: false,
            resource: new Resource({
                [SemanticResourceAttributes.SERVICE_NAME]: extensionContext.extension.packageJSON.name,
                [SemanticResourceAttributes.SERVICE_VERSION]: extensionContext.extension.packageJSON.version,
                [SemanticResourceAttributes.SERVICE_NAMESPACE]:
                    (env.appName == "Visual Studio Code" ? "vscode" : env.appName) + "." + env.appHost,
                [SemanticResourceAttributes.SERVICE_INSTANCE_ID]: env.sessionId,
                [SemanticResourceAttributes.DEVICE_ID]: env.machineId.slice(0, 8),
            })
        });
        const contextManager = new AsyncHooksContextManager();
        contextManager.enable();
        api.context.setGlobalContextManager(contextManager);

        if (env.isTelemetryEnabled) {
            this.sdk.start();
        }

What I'm saying is that I am not skilled at opentelemetry implementations, and must rely heavily on documentation and examples. I'm not able to find the documentation and example I need to adapt my cut-and-pasted implementation.

I'm certain that in a few weeks, if I should need to dive deeper into the internals of opentelemetry, the answer to my question will be obvious. But that answer is not obvious today, and diving deep into learning opentelemetry is daunting because of the apparently frequent API changes make it hard to know what's current and what is not (which means Google's and Stackoverflow's answers are frustratingly outdated).

Would you kindly point me precisely to a js/ts snippet that I might be able to easily adapt without in-depth knowledge of opentelemetry? I'm not dumb, but I don't have the time or inclination for the requisite deep dive. The description of the setRecords is taunting me as if I should be able to just know how to implement it, but it requires climbing a learning curve I have not yet climbed.

TLDR, yes. I read the entire thread.

@drok
Copy link

drok commented Nov 28, 2023

Re: java vs javascript. The reason I comment here, although I'm looking for a js solution, is that Googling "how to remove default opentelemetry.sdk resources" doesn't lead anywhere else that seems more relevant. This Java implementation is the closest place that I can find where skipping non-default Records is mentioned.

It seems everyone else who uses OPTL doesn't care about the unneeded attributes being using up bandwidth.

Please do show a full Java example. I can try ChatGPT to translate it into the equivalent javascript incantation, although ChatGPT does not have info about the new API function (re)names.

@trask
Copy link
Member

trask commented Nov 28, 2023

In Java it's just setResource(Resources.empty())

@jkwatson
Copy link
Contributor

@drok Is there some reason you aren't asking this in the javascript repository/slack? There is very little chance that ChatGPT will be able to do any translation of APIs between the javascript SDK and the java SDK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants