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

Add code attributes to dropwizard views #12984

Closed

Conversation

jaydeluca
Copy link
Member

Related to #7345

@jaydeluca jaydeluca requested a review from a team as a code owner December 31, 2024 23:16
@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!)

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.

4 participants