Skip to content

feat: Add otel spans to tool calls #206

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

Jump to bottom
Merged
merged 14 commits into from
Jul 23, 2025
Merged

feat: Add otel spans to tool calls #206

merged 14 commits into from
Jul 23, 2025

Conversation

SandersAaronD
Copy link
Contributor

  • Adds trace spans for o11y into the call chain
  • Tool arguments are not logged by default to prevent accidental PII exposure, although this seems like it may be excessively cautious and might hinder debugging especially
  • We should be able to simply set this to log tool calls when we want to
  • Also fixes test environment isolation issues in TestExtractGrafanaInfoFromHeaders that was causing failures when GRAFANA_API_KEY environment variables were present.

@SandersAaronD SandersAaronD requested a review from a team as a code owner July 17, 2025 05:14
annanay25
annanay25 reviewed Jul 17, 2025
View reviewed changes
Copy link
Contributor

@annanay25 annanay25 left a comment

Choose a reason for hiding this comment

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

agree overall to more instrumentation, just concerned about the LogTraceArguments option.

sd2k
sd2k reviewed Jul 18, 2025
View reviewed changes
Copy link
Collaborator

@sd2k sd2k left a comment

Choose a reason for hiding this comment

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

This does look helpful, I think we should unconditionally create spans and propagate the context though?

Perhaps we should use the flag to determine whether to instantiate an OTel exporter in our CLI?

@SandersAaronD
Copy link
Contributor Author

I think we've got this addressed and we have span creation by default now with a cleaner contract, if there's anything still outstanding I can throw those changes on too.

sd2k
sd2k reviewed Jul 19, 2025
View reviewed changes
Copy link
Collaborator

@sd2k sd2k left a comment

Choose a reason for hiding this comment

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

The reflection is a bit odd but I guess it does save an import!

SandersAaronD and others added 2 commits July 22, 2025 11:41
@SandersAaronD @sd2k
Update tools.go
e792cc0 
Co-authored-by: Ben Sully <ben.sully@grafana.com>
@SandersAaronD
Tidy up around otel behavior
5872996
@SandersAaronD
Copy link
Contributor Author

@sd2k commit + some tidy up for testing and the extra nil checks are done, this should be good to go

@SandersAaronD SandersAaronD enabled auto-merge (squash) July 22, 2025 19:03
sd2k
sd2k approved these changes Jul 23, 2025
View reviewed changes
Copy link
Collaborator

@sd2k sd2k left a comment

Choose a reason for hiding this comment

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

Nice, ty!

@sd2k sd2k disabled auto-merge July 23, 2025 15:48
@sd2k sd2k merged commit a3ae12b into main Jul 23, 2025
18 of 19 checks passed
@sd2k sd2k deleted the sandersaarond/add-tracing branch July 23, 2025 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annanay25 annanay25 annanay25 left review comments

@sd2k sd2k sd2k approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SandersAaronD @sd2k @annanay25