Skip to content

patch: set span name as tag#126

Merged
alexopenline merged 1 commit into
otterfrom
span-name
May 10, 2025
Merged

patch: set span name as tag#126
alexopenline merged 1 commit into
otterfrom
span-name

Conversation

@alexopenline
Copy link
Copy Markdown
Contributor

@alexopenline alexopenline commented May 10, 2025

Important

Set span name as an attribute in OpenTelemetry spans in helpers.go.

  • Telemetry Enhancement:
    • In helpers.go, StartSpan() now sets the span name as an attribute name in OpenTelemetry spans.

This description was created by Ellipsis for db766f8. You can customize this summary. It will automatically update as commits are pushed.

@alexopenline alexopenline marked this pull request as ready for review May 10, 2025 07:23
@alexopenline alexopenline merged commit 6a902c5 into otter May 10, 2025
3 checks passed
@alexopenline alexopenline deleted the span-name branch May 10, 2025 07:24
Copy link
Copy Markdown

@ellipsis-dev ellipsis-dev Bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to db766f8 in 1 minute and 12 seconds. Click for details.
  • Reviewed 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. internal/telemetry/helpers.go:102
  • Draft comment:
    Consider using a more descriptive attribute key than "name" (e.g., "span.name") to better align with OpenTelemetry semantic conventions and avoid potential ambiguity with the span's operation name.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. internal/telemetry/helpers.go:102
  • Draft comment:
    Setting the span name as an attribute ('name') might be redundant since the span's operation name is already set via tracer.Start. Consider if this extra attribute is needed or if you want to use a standard semantic attribute key. Also, if you keep it, consider grouping it with other attributes.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment makes a valid point about redundancy - storing the same information twice doesn't add value. The suggestion about grouping attributes is also reasonable since there are multiple SetAttributes calls that could be combined. However, this is more of a style/optimization suggestion rather than a clear bug or issue that needs fixing. The redundant attribute might actually be intentional for easier querying or visualization in the telemetry system. Without knowing the full telemetry setup and requirements, removing it could break existing dashboards or queries. While the redundancy might be intentional, the comment still provides value by making the author think about whether this duplication is really needed and suggesting a potential optimization with attribute grouping. The comment raises valid points about code optimization, but since it's not pointing out a clear bug and requires more context about telemetry requirements to make a decision, it's better to err on the side of not keeping it.

Workflow ID: wflow_zfHrpFPVOK1Abdeo

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

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.

1 participant