-
Notifications
You must be signed in to change notification settings - Fork 325
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
added support for adding span destination service info to public api #1551
added support for adding span destination service info to public api #1551
Conversation
❕ Build Aborted
Expand to view the summary
Build stats
Steps errorsExpand to view the steps failures
|
|
||
@Nonnull | ||
@Override | ||
public Span withDestinationServiceResource(String resource) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I think we're hitting some limits with how we modelled spans and transactions here. These methods are only relevant for spans, not transactions. But in the public API, transaction extends span. We should think about introducing a BaseSpan
that contains methods that are relevant for both spans and transactions. But that would likely break binary compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @felixbarny , please review changes
apm-agent-api/src/main/java/co/elastic/apm/api/AbstractSpanImpl.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #1551 +/- ##
============================================
- Coverage 59.00% 58.98% -0.03%
- Complexity 92 2309 +2217
============================================
Files 391 391
Lines 17622 17615 -7
Branches 2441 2436 -5
============================================
- Hits 10398 10390 -8
- Misses 6505 6508 +3
+ Partials 719 717 -2
Continue to review full report at Codecov.
|
@kananindzya I am closing this as I believe this is superseded by #1788 . |
closes #1521