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 #1574: [Bug] - Span attributes not added when obfuscator is enabled #1575

Conversation

heiko-holz
Copy link
Contributor

@heiko-holz heiko-holz commented Feb 8, 2023

Closes #1574


This change is Reviewable

@heiko-holz heiko-holz marked this pull request as ready for review February 8, 2023 09:41
@heiko-holz heiko-holz force-pushed the fixes/fix-1574-exception-span-attribute-cast-exception branch from 4269c36 to 90aa908 Compare February 8, 2023 10:02
Copy link
Contributor

@quandor quandor left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @heiko-holz)


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/OtlpTraceExporterService.java line 66 at r1 (raw file):

            OtlpTraceExporterSettings otlp = configuration.getExporters().getTracing().getOtlp();

            String endpoint = otlp.getEndpoint();

I saw that code in three different places so far. Is it possible to move that into otlp.getEndpoint()? In that way we would have easily at a single place.


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/ExporterServiceIntegrationTestBase.java line 297 at r1 (raw file):

                }

                return (childSpan.get().getParentSpanId().equals(parentSpan.get().getSpanId()));

Those enclosing parenthesis are unnecessary, or am I missing something?


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/privacy/obfuscation/ObfuscationManagerTest.java line 294 at r1 (raw file):

            verifyNoMoreInteractions(span);

            // TODO: Boolean

Das TODO sieht für mich erledigt aus ;-)

@heiko-holz
Copy link
Contributor Author

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/OtlpTraceExporterService.java line 66 at r1 (raw file):

Previously, quandor (Jochen Just) wrote…

I saw that code in three different places so far. Is it possible to move that into otlp.getEndpoint()? In that way we would have easily at a single place.

Yeah, good idea.
I agree.

However, I'd first like to talk to you about the "http-padding" that I introduced.

@heiko-holz
Copy link
Contributor Author

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/ExporterServiceIntegrationTestBase.java line 297 at r1 (raw file):

Previously, quandor (Jochen Just) wrote…

Those enclosing parenthesis are unnecessary, or am I missing something?

Thanks for spotting.
I removed the redundant parentheses.

Copy link
Contributor Author

@heiko-holz heiko-holz 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: 3 of 14 files reviewed, 3 unresolved discussions (waiting on @quandor)


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/privacy/obfuscation/ObfuscationManagerTest.java line 294 at r1 (raw file):

Previously, quandor (Jochen Just) wrote…

Das TODO sieht für mich erledigt aus ;-)

Yes, indeed.
I left it to see if the reviewer sees it ;) /joking

Copy link
Contributor

@quandor quandor left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 11 of 11 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @heiko-holz)


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/OtlpTraceExporterService.java line 66 at r1 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

Yeah, good idea.
I agree.

However, I'd first like to talk to you about the "http-padding" that I introduced.

LGTM


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/ExporterServiceIntegrationTestBase.java line 297 at r1 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

Thanks for spotting.
I removed the redundant parentheses.

👍

@quandor quandor merged commit 9d97570 into inspectIT:master Feb 9, 2023
@heiko-holz heiko-holz added the bug Something isn't working label Feb 9, 2023
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

Successfully merging this pull request may close these issues.

[Bug] - Span attributes not added when obfuscator is enabled
2 participants