Auto-infer destination.service.resource and adapt public API #1520
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Test errors
Expand to view the tests failures> Show only the first 10 test failures
|
russcam
reviewed
Oct 20, 2021
| @@ -125,7 +125,8 @@ public interface IExecutionSegment | |||
| /// </param> | |||
| /// <param name="subType">The subtype of the span.</param> | |||
| /// <param name="action">The action of the span.</param> | |||
| void CaptureSpan(string name, string type, Action<ISpan> capturedAction, string subType = null, string action = null); | |||
| /// <param name="isExitSpan">Indicates if this span is an exit span.</param> | |||
| void CaptureSpan(string name, string type, Action<ISpan> capturedAction, string subType = null, string action = null, bool isExitSpan = false); | |||
There was a problem hiding this comment.
These are technically breaking changes to a public API, however this is probably the simplest way to implement exit spans. I can't think of a cleaner alternative
There was a problem hiding this comment.
Good point. Yeah, I also think this is the best option we have.
russcam
approved these changes
Oct 20, 2021
There was a problem hiding this comment.
The changes LGTM. A couple of questions:
-
Should a property be included on
ISpanto indicate whether it is an exit span? -
It looks like exit span can be used in places where
InstrumentationFlagis currently used to suppress child spans, such as in HttpDiagnosticListenerImplBaseShould this be part of this PR?
- Introduces the concept of exit spans - Calculate Span.Context.Destination.Service.Resource only for exit spans based on the spec - Adapt all instrumentation to set the isExitSpan flag - Remove usage of the obsolete fields Destination.Service.Name and Destination.Service.Type
|
|
Merging this in - there are some failing tests with |
4 tasks
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Test errors
Expand to view the tests failures> Show only the first 10 test failures
|
russcam
reviewed
Oct 21, 2021
| /// <summary> | ||
| /// Indicates that this span is an exit span. | ||
| /// </summary> | ||
| public bool IsExitSpan { get; } |
There was a problem hiding this comment.
I think the public modifier is unintentional here (will be public by default)?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Solves #1330
Destination.Service.NameandDestination.Service.Type- these fields were never used in APM Server and we should remove those long term according to [META 448] Auto-infer destination.service.resource and adapt public API #1330Destination.Service.Resourcefor exit spans.Destination.Service.Resourcefor exit spans. According to the spec this should only happen for exit spans which was not the case prior to this PRStartSpanandCaptureSpanAPIs to also start exit spans and adapts the usage of it in instrumentation logic.