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

Closes #1461 - Add OTEL Version to Trace Exporter #1464

Merged
merged 13 commits into from
Jun 9, 2022
Merged

Conversation

MariusBrill
Copy link
Member

@MariusBrill MariusBrill commented Jun 7, 2022

This change is Reviewable

@mariusoe mariusoe self-assigned this Jun 7, 2022
@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #1464 (62eaf25) into master (bcf0d1c) will decrease coverage by 0.03%.
The diff coverage is 81.25%.

@@             Coverage Diff              @@
##             master    #1464      +/-   ##
============================================
- Coverage     79.05%   79.02%   -0.03%     
+ Complexity     2228     2225       -3     
============================================
  Files           232      228       -4     
  Lines          7252     7237      -15     
  Branches        860      859       -1     
============================================
- Hits           5733     5719      -14     
  Misses         1160     1160              
+ Partials        359      358       -1     
Impacted Files Coverage Δ
...in/java/rocks/inspectit/ocelot/core/AgentImpl.java 68.52% <40.00%> (-2.91%) ⬇️
...rocks/inspectit/ocelot/bootstrap/AgentManager.java 66.67% <100.00%> (+4.17%) ⬆️
...ore/opentelemetry/OpenTelemetryControllerImpl.java 78.53% <100.00%> (+0.40%) ⬆️
...nstrumentation/actions/GenericActionGenerator.java 97.30% <0.00%> (-0.14%) ⬇️
...s/bound/NonVoidConstantOnlyBoundGenericAction.java
...ions/bound/VoidConstantOnlyBoundGenericAction.java
.../bound/AbstractConstantOnlyBoundGenericAction.java
...n/actions/bound/VoidDynamicBoundGenericAction.java
...tions/bound/AbstractDynamicBoundGenericAction.java
...ctions/bound/NonVoidDynamicBoundGenericAction.java
... and 4 more

Copy link
Member

@mariusoe mariusoe left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 8 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 7 of 8 files reviewed, 8 unresolved discussions (waiting on @MariusBrill and @mariusoe)


inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/AgentManager.java line 89 at r2 (raw file):

     * @return the current OTEL version
     */
    public static String getOtelVersion() {

Please use getOpenTelemetryVersion.

Actually, we don't need this method. We can just use getAgent().getOpenTelemetryVersion()..?

Code quote:

getOtelVersion(

inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/IAgent.java line 33 at r2 (raw file):

    /**
     * Returns the version of Open Telemetry the agent was build with.

without space OpenTelemetry

Code quote:

Open Telemetry

inspectit-ocelot-core/build.gradle line 201 at r2 (raw file):

        def currentDate = new Date().toString()
        ext.versionFile.withWriter('UTF-8') { writer ->
            writer << "$version\nOpenTelemetry $openTelemetryVersion\n$currentDate"

Remove this. Just insert the OTel version. We have to know, that the second row is the OTel version, thus, we don't need this prefix

Code quote:

OpenTelemetry 

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/AgentImpl.java line 64 at r2 (raw file):

     * The version of open telemetry that was used for building the agent.
     */
    private String otelVersion;

Please use the full name openTelemetryVersion

Code quote:

otelVersion

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/AgentImpl.java line 127 at r2 (raw file):

            LOGGER.warn("Could not read agent version information file.");
            agentVersion = "UNKNOWN";
            agentBuildDate = "UNKNOWN";

Set openTelemetry to UNKOWN as well in case of an error.


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/config/propertysources/EnvironmentInformationPropertySource.java line 30 at r2 (raw file):

        result.put("inspectit.env.agent-dir", getAgentJarDirectory());
        result.put("inspectit.env.agent-version", AgentManager.getAgentVersion());
        result.put("inspectit.env.open-telemetry-version", AgentManager.getOtelVersion());

Let's remove this because we don't need it at the moment. We can still add it in case we need it at some point in time..


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 54 at r2 (raw file):

    public static final String BEAN_NAME = "openTelemetryController";

    public static final String INSPECTIT_ENV_OTEL_VERSION_PATH = "inspectit.env.otel-version";

Not needed. Can be removed


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 339 at r2 (raw file):

        multiSpanExporter = DynamicMultiSpanExporter.create();
        Resource tracerProviderAttributes = Resource.create(Attributes.of(
                ResourceAttributes.SERVICE_NAME, configuration.getExporters().getTracing().getServiceName(),

This block is not formatted using the formatter. Please format.

You canuse

// @formatter:off
...
// @formatter:on 

in case you want to preserve some formatting/indentation in these lines.

Code quote:

                ResourceAttributes.SERVICE_NAME, configuration.getExporters().getTracing().getServiceName(),
                AttributeKey.stringKey("inspectit.agent.version"),AgentManager.getAgentVersion(),
                ResourceAttributes.TELEMETRY_SDK_VERSION, AgentManager.getOtelVersion()

Copy link
Member Author

@MariusBrill MariusBrill left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 8 files reviewed, 8 unresolved discussions (waiting on @mariusoe)


inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/IAgent.java line 33 at r2 (raw file):

Previously, mariusoe (Marius Oehler) wrote…

without space OpenTelemetry

Done.


inspectit-ocelot-core/build.gradle line 201 at r2 (raw file):

Previously, mariusoe (Marius Oehler) wrote…

Remove this. Just insert the OTel version. We have to know, that the second row is the OTel version, thus, we don't need this prefix

Without this the end result in the traces would be "telemetry.sdk.version: XZY". How about then adding a new attribute like openlemetry.version to make it clear which standard the agent uses?


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/AgentImpl.java line 64 at r2 (raw file):

Previously, mariusoe (Marius Oehler) wrote…

Please use the full name openTelemetryVersion

Done.


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/AgentImpl.java line 127 at r2 (raw file):

Previously, mariusoe (Marius Oehler) wrote…

Set openTelemetry to UNKOWN as well in case of an error.

Done.


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 54 at r2 (raw file):

Previously, mariusoe (Marius Oehler) wrote…

Not needed. Can be removed

Done.


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/opentelemetry/OpenTelemetryControllerImpl.java line 339 at r2 (raw file):

Previously, mariusoe (Marius Oehler) wrote…

This block is not formatted using the formatter. Please format.

You canuse

// @formatter:off
...
// @formatter:on 

in case you want to preserve some formatting/indentation in these lines.

Done.


inspectit-ocelot-bootstrap/src/main/java/rocks/inspectit/ocelot/bootstrap/AgentManager.java line 89 at r2 (raw file):

Previously, mariusoe (Marius Oehler) wrote…

Please use getOpenTelemetryVersion.

Actually, we don't need this method. We can just use getAgent().getOpenTelemetryVersion()..?

Done.


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/config/propertysources/EnvironmentInformationPropertySource.java line 30 at r2 (raw file):

Previously, mariusoe (Marius Oehler) wrote…

Let's remove this because we don't need it at the moment. We can still add it in case we need it at some point in time..

Done.

Copy link
Member

@mariusoe mariusoe left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 5 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @MariusBrill)

@mariusoe mariusoe merged commit 2fc9821 into master Jun 9, 2022
@mariusoe mariusoe deleted the fix/fix-1461 branch June 9, 2022 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] - OTEL Trace Exporters do not show OTEL version as attribute in traces
2 participants