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

Add code attributes to dropwizard views #12984

Closed
wants to merge 1 commit into from
Closed

Add code attributes to dropwizard views #12984

Show file tree
Hide file tree
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
2 changes: 2 additions & 0 deletions ...java/io/opentelemetry/javaagent/instrumentation/dropwizardviews/DropwizardSingletons.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import io.dropwizard.views.View;
import io.opentelemetry.api.GlobalOpenTelemetry;
import io.opentelemetry.instrumentation.api.incubator.semconv.code.CodeAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
import io.opentelemetry.javaagent.bootstrap.internal.ExperimentalConfig;

Expand All @@ -17,6 +18,7 @@ public final class DropwizardSingletons {
private static final Instrumenter<View, Void> INSTRUMENTER =
Instrumenter.<View, Void>builder(
GlobalOpenTelemetry.get(), INSTRUMENTATION_NAME, DropwizardSingletons::spanName)
.addAttributesExtractor(CodeAttributesExtractor.create(new ViewCodeAttributesGetter()))
.setEnabled(ExperimentalConfig.get().controllerTelemetryEnabled())
.buildInstrumenter();

Expand Down
25 changes: 25 additions & 0 deletions .../io/opentelemetry/javaagent/instrumentation/dropwizardviews/ViewCodeAttributesGetter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.javaagent.instrumentation.dropwizardviews;

import io.dropwizard.views.View;
import io.opentelemetry.instrumentation.api.incubator.semconv.code.CodeAttributesGetter;
import javax.annotation.Nullable;

public class ViewCodeAttributesGetter implements CodeAttributesGetter<View> {

@Nullable
@Override
public Class<?> getCodeClass(View view) {
return view.getClass();
}

@Nullable
@Override
public String getMethodName(View view) {
return "render";
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is appropriate. While the language in https://opentelemetry.io/docs/specs/semconv/general/attributes/#source-code-attributes does not explicitly disallow such usage in my opinion it still suggests that code attributes should correspond to actual code that was executed for the given span. Since the render method does't exist in the view class with given usage these attributes do not represent actual code. render method exists in the ViewRenderer class and this is where the span is created, but the question is would the code attribute be useful there? Would knowing whether view was rendered by MustacheViewRenderer or FreemarkerViewRenderer be helpful?
To proceed with this PR in its current for a confirmation is needed that such usage is allowed by the semantic conventions. cc @trask

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's a good point to add for clarification in open-telemetry/semantic-conventions#1677. In this context it seems to me that the view class (which is put in code.namespace) is enough for the end-user to identify what is being executed in this view, without needing to introduce an artificial value for code.function.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this context it seems to me that the view class (which is put in code.namespace) is enough for the end-user to identify what is being executed in this view, without needing to introduce an artificial value for code.function.

Although the definition of code.namespace uses code.function in https://github.com/open-telemetry/semantic-conventions/blob/f0db7751ab3facf2bd1261bb24748fbdb7931c20/docs/attributes-registry/code.md?plain=1#L19 which sort of implies that the when namespace is set function should also be set I think leaving out code.function is fine. My concern is that https://opentelemetry.io/docs/specs/semconv/general/attributes/#source-code-attributes states

Often a span is closely tied to a certain unit of code that is logically responsible for handling the operation that the span describes (usually the method that starts the span).

The View class is not really responsible for creating the span, it is ViewRenderer.render instead. The view just provides inputs like template name and charset to the renderer. Since the view class is only loosely related to the span my concern is whether using it in the code.namespace attribute is appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that making the code.namespace and code.function definitions avoid referring to each-other could help remove the implicit dependency here:

  • code.namespace could be used to indicate a class name, for example here with the view class
  • code.function could be used even when code.namespace is not set, which is already the case when a global function is used.

On the fact that the View class here is not really responsible for creating the span, I don't have any strong opinion, do you have ideas how we could better store this information ? For example, if we had an mvc.view or template.name in semantic conventions would that be a better choice than those code-related semconv attributes ?

Copy link
Member

Choose a reason for hiding this comment

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

@laurit good points

I think I'd prefer to stay conservative at this point until we have more semconv guidance, and stick to using code.function on spans which represent the execution of that function.

For example, if we had an mvc.view or template.name in semantic conventions would that be a better choice than those code-related semconv attributes ?

yeah, this makes sense to me, since view names don't always correspond to classes / functions

@jaydeluca I've updated #7345 for now to exclude view spans (thanks for sending this though, I hadn't thought about these issues!)

}
}
34 changes: 26 additions & 8 deletions .../test/java/io/opentelemetry/javaagent/instrumentation/dropwizardviews/ViewRenderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,16 @@

package io.opentelemetry.javaagent.instrumentation.dropwizardviews;

import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo;
import static io.opentelemetry.semconv.incubating.CodeIncubatingAttributes.CODE_FUNCTION;
import static io.opentelemetry.semconv.incubating.CodeIncubatingAttributes.CODE_NAMESPACE;
import static org.assertj.core.api.Assertions.assertThat;

import io.dropwizard.views.View;
import io.dropwizard.views.ViewRenderer;
import io.dropwizard.views.freemarker.FreemarkerViewRenderer;
import io.dropwizard.views.mustache.MustacheViewRenderer;
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension;
import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
Expand Down Expand Up @@ -38,22 +42,36 @@ private static Stream<Arguments> provideParameters() {
Arguments.of(new MustacheViewRenderer(), "/views/mustache/utf8.mustache"));
}

// Used to avoid the View class being instantiated as an anonymous inner class, which results in
// the class name being ViewRenderTest$1 instead of ViewRenderTest when asserting CODE_NAMESPACE.
private static class TestView extends View {
protected TestView(String templateName) {
super(templateName, StandardCharsets.UTF_8);
}
}

@ParameterizedTest
@MethodSource("provideParameters")
void testSpan(ViewRenderer renderer, String template) throws IOException {
View view = new View(template, StandardCharsets.UTF_8) {};
View view = new TestView(template);
ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
testing.runWithSpan(
"parent",
() -> {
renderer.render(view, Locale.ENGLISH, outputStream);
});
testing.runWithSpan("parent", () -> renderer.render(view, Locale.ENGLISH, outputStream));

assertThat(outputStream.toString("UTF-8")).contains("This is an example of a view");
testing.waitAndAssertTraces(
trace ->
trace.hasSpansSatisfyingExactly(
span -> span.hasName("parent").hasKind(SpanKind.INTERNAL).hasNoParent(),
span -> span.hasName("Render " + template).hasParent(trace.getSpan(0))));
span ->
span.hasName("parent")
.hasKind(SpanKind.INTERNAL)
.hasNoParent()
.hasAttributes(Attributes.empty()),
span ->
span.hasName("Render " + template)
.hasParent(trace.getSpan(0))
.hasAttributesSatisfyingExactly(
equalTo(CODE_FUNCTION, "render"),
equalTo(CODE_NAMESPACE, TestView.class.getName()))));
}

@Test
Expand Down
Loading