Refactor AwsMetricAttributesSpanExporter to a processor #1250
Conversation
majanjua-amzn
force-pushed
the
refactor-exporter
branch
3 times, most recently
from
7167e8c to
0e1a512
Compare
majanjua-amzn
force-pushed
the
refactor-exporter
branch
from
0e1a512 to
f6008fb
Compare
| // Construct and set local and remote attributes span processor | ||
| tracerProviderBuilder.addSpanProcessor( | ||
| AttributePropagatingSpanProcessorBuilder.create().build()); | ||
| // Construct and set AWS Attribute Generating Span Processor |
There was a problem hiding this comment.
Maybe comment that the order of span processors matters. I actually wonder if it would be better to have some kind of explicit enforcement of this.
| import io.opentelemetry.sdk.trace.ReadWriteSpan; | ||
| import io.opentelemetry.sdk.trace.ReadableSpan; | ||
| import io.opentelemetry.sdk.trace.data.SpanData; | ||
| import io.opentelemetry.sdk.trace.internal.ExtendedSpanProcessor; |
There was a problem hiding this comment.
I forgot that OnEnding is in development, forcing us to depend on internal components explicitly not for public usage. I just want to take a moment and think about this before committing hard on this. I think fundamentally the functionality we are using is not complex and unlikely to just fundamentally change or be reverted, but development features are explicitly prone to that.
| SpanData spanData = span.toSpanData(); | ||
| Map<String, Attributes> attributeMap = | ||
| generator.generateMetricAttributeMapFromSpan(span.toSpanData(), resource); | ||
|
|
||
| boolean generatesServiceMetrics = |
There was a problem hiding this comment.
| SpanData spanData = span.toSpanData(); | |
| Map<String, Attributes> attributeMap = | |
| generator.generateMetricAttributeMapFromSpan(span.toSpanData(), resource); | |
| boolean generatesServiceMetrics = | |
| SpanData spanData = span.toSpanData(); | |
| Map<String, Attributes> attributeMap = | |
| generator.generateMetricAttributeMapFromSpan(spanData, resource); | |
| boolean generatesServiceMetrics = |
| .thenReturn(attributeMap); | ||
|
|
||
| // End span and verify it was modified appropriately | ||
| span.end(); |
There was a problem hiding this comment.
Curious - how does onEnding get called?
| } | ||
|
|
||
| @Test | ||
| public void testOnEndingGeneratesBothWithoutOverride() { |
There was a problem hiding this comment.
Not blocking - a couple comments would not go amiss - e.g. why is this span a local root? What is/isn't being overridden? Both what?
TBH I find the name confusing anyways, so if not a comment, maybe a new name - same for the rest of these tests.
|
|
||
| @Test | ||
| public void testOnEndingGeneratesService() { | ||
| Span span = tracer.spanBuilder("test").setSpanKind(SpanKind.SERVER).startSpan(); |
There was a problem hiding this comment.
Have at least two span attribute (one replaced, one unchanged) to show we don't just completely replace attributes, etc. Same with testOnEndingGeneratesDependency
| } | ||
|
|
||
| @Test | ||
| public void testOnEndingGeneratesService() { |
There was a problem hiding this comment.
| public void testOnEndingGeneratesService() { | |
| public void testOnEndingGeneratesServiceAttributes() { |
etc.
| // Ensure span is treated as only having dependency metric information | ||
| try (MockedStatic<AwsSpanProcessingUtil> mockUtil = | ||
| Mockito.mockStatic(AwsSpanProcessingUtil.class)) { | ||
| mockUtil | ||
| .when( | ||
| () -> | ||
| AwsSpanProcessingUtil.shouldGenerateServiceMetricAttributes(any(SpanData.class))) | ||
| .thenReturn(false); | ||
| mockUtil | ||
| .when( | ||
| () -> | ||
| AwsSpanProcessingUtil.shouldGenerateDependencyMetricAttributes( | ||
| any(SpanData.class))) | ||
| .thenReturn(true); |
There was a problem hiding this comment.
This is fine, but you could also probably just have a parent and child span, then audit the child span only.
| import org.mockito.MockitoAnnotations; | ||
|
|
||
| /** Unit tests for {@link AwsAttributeGeneratingSpanProcessor}. */ | ||
| class AwsAttributeGeneratingSpanProcessorTest { |
There was a problem hiding this comment.
Probably for all tests: we should include both DEPENDENCY_METRIC and SERVICE_METRIC in attributeMap, to show that only one is ever used. Actually ideally we would do something where if we expect only DEPENDENCY_METRIC to be used, then if we attempt to pull SERVICE_METRIC, we throw an exception and fail the test. Or something.
| if (generatesServiceMetrics && generatesDependencyMetrics) { | ||
| // Order matters: dependency metric attributes include AWS_SPAN_KIND key | ||
| span.setAllAttributes(attributeMap.get(MetricAttributeGenerator.DEPENDENCY_METRIC)); | ||
| span.setAttribute(AWS_SPAN_KIND, AwsSpanProcessingUtil.LOCAL_ROOT); | ||
| } else if (generatesServiceMetrics) { | ||
| span.setAllAttributes(attributeMap.get(MetricAttributeGenerator.SERVICE_METRIC)); | ||
| } else if (generatesDependencyMetrics) { | ||
| span.setAllAttributes(attributeMap.get(MetricAttributeGenerator.DEPENDENCY_METRIC)); | ||
| } |
There was a problem hiding this comment.
Consideration: If (for example) attributeMap does NOT contain SERVICE_METRIC and we call span.setAllAttributes(attributeMap.get(MetricAttributeGenerator.SERVICE_METRIC)), what happens? If it throws an exception or something, then that is a problem, because onEnding is now in application path (where before export was, IIRC, async). We may need to do some defensive programming here and log debugs if expectations are not met.
2 participants
Background
With the addition of
onEndingas part of the ExtendedSpanProcessor in OTel core (open-telemetry/opentelemetry-java#6367), we are now able to modify spans before they are sent to an exporter. As such, we have the opportunity to refactor ourAwsMetricAttributesSpanExporterwhich is responsible for adding service and dependency metric related attributes to spans as they are exported. This was done via some complicated span wrapping logic which is no longer necessary withonEnding.Changes
AwsMetricAttributesSpanExporterwithAwsAttributeGeneratingSpanProcessorAwsMetricAttributesSpanExportertoAwsAttributeGeneratingSpanProcessor, removing any exporter-related functionality and moving the span/attribute modification logic toprocessor.onEnding()AwsMetricAttributesSpanExporterBuilderwith theAwsAttributeGeneratingSpanProcessorBuilderTesting
Converted most of the exporter tests into equivalent tests for the new processor:
testPassthroughDelegationstestExportDelegatingSpanDataBehaviourtestExportDelegationWithMultipleSpanstestExportDelegationWithoutAttributeOrModification→testOnEndingGeneratesBothWithoutOverridetestExportDelegationWithAttributeButWithoutModification→testOnEndingGeneratesServicetestExportDelegationWithoutAttributeButWithModification→testOnEndingGeneratesBothWithoutOverridetestExportDelegationWithAttributeAndModification→testOnEndingGeneratesBothWithOverridetestOverridenAttributes→testOnEndingGeneratesBothWithOverridetestExportDelegationWithTwoMetrics→testOnEndingGeneratesBothWithTwoMetricstestConsumerProcessSpanHasEmptyAttribute→testConsumerSpanHasEmptyAttributestestExportDelegationWithDependencyMetrics→testOnEndingGeneratesDependencyAlso ran the entire main build workflow using this change successfully:
https://github.com/aws-observability/aws-otel-java-instrumentation/actions/runs/18785649764
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.