Rename AgentSpan.context() to AgentSpan.spanContext() to disambiguate it from generic context#11707
Rename AgentSpan.context() to AgentSpan.spanContext() to disambiguate it from generic context#11707mcculls wants to merge 4 commits into
AgentSpan.context() to AgentSpan.spanContext() to disambiguate it from generic context#11707Conversation
… it from generic context
mcculls
added
comp: core
mcculls
requested review from
PerfectSlayer,
amarziali,
daniel-romano-DD,
jandro996,
mtoffl01,
tylfin and
ygree
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2b4698ede
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The rename is complete and fully consistent across all 142 changed files. A comprehensive grep across the entire repo found zero remaining callers of the old AgentSpan.context() signature in production source. The key disambiguation — AgentScope.context() returns Context (the framework context object) while AgentSpan.spanContext() returns AgentSpanContext (the tracing span context) — is preserved correctly, and the OpenTracing public API Span.context() is untouched.
🤖 Datadog Autotest · Commit c2b4698 · What is Autotest? · @autotest to ask questions · Any feedback? Reach out in #autotest
PerfectSlayer
left a comment
PerfectSlayer
left a comment
There was a problem hiding this comment.
Make sense. Should we rename some of the variable names too? (I added comments for the most of them I think).
💭 thought: At some point, I wonder if we should not rename ExtractedContext to ExtractedSpanContext or RemoteSpanContext.
Debugger benchmarksParameters
See matching parameters
SummaryFound 0 performance improvements and 5 performance regressions! Performance is the same for 5 metrics, 5 unstable metrics.
See unchanged results
Request duration reports for reportsgantt
title reports - request duration [CI 0.99] : candidate=None, baseline=None
dateFormat X
axisFormat %s
section baseline
noprobe (332.954 µs) : 305, 361
. : milestone, 333,
basic (296.438 µs) : 290, 303
. : milestone, 296,
loop (8.981 ms) : 8976, 8986
. : milestone, 8981,
section candidate
noprobe (341.042 µs) : 305, 377
. : milestone, 341,
basic (298.673 µs) : 291, 306
. : milestone, 299,
loop (9.401 ms) : 9281, 9522
. : milestone, 9401,
|
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
amarziali
left a comment
amarziali
left a comment
There was a problem hiding this comment.
The renaming sounds reasonable to me
|
I must admit reviewing the changes and getting reminded about the actual use for the "span context" is quite depressing. I hove at some point we will succeed in trimming it - so it is no longer the main data model holder for so many features 😓 |
Kafka / producer-benchmarkParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 3 metrics, 0 unstable metrics. See unchanged results
|
This comment has been minimized.
This comment has been minimized.
Kafka / consumer-benchmarkParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 3 metrics, 0 unstable metrics. See unchanged results
|
mcculls
force-pushed
the
mcculls/disambiguate-agent-span-context
branch
3 times, most recently
from
a066108 to
1759a57
Compare
mcculls
force-pushed
the
mcculls/disambiguate-agent-span-context
branch
from
1759a57 to
281ce7d
Compare
mcculls
force-pushed
the
mcculls/disambiguate-agent-span-context
branch
from
639f4b3 to
b43621b
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in Use ⏳ Processing |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
5 participants
Motivation
spanContext()more accurately describes what it returns, which is aSpanContextAgentSpanis an internal API so this is not a breaking change for external users.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]