apm: add ParentID methods to Span and Transaction #956
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
Adds two new methods to the `Span` and `Transaction` types, expanding the API so consumers can extract the ParentIDs should they desire to. The `Transaction.ParentID()` call reuses the already existing method `Transaction.EnsureParent()` since it handles all the cases already. `Span.ParentID()` is much simpler and does not generate a new SpanID when `parentID` is zero or nil, simply returns an empty `SpanID` with its zero value. Closes #909. Signed-off-by: Marc Lopez <marc5.12@outlook.com>
axw
requested changes
Apr 23, 2021
There was a problem hiding this comment.
Thanks @marclop! Looks good overall, but I'd like to request a couple of main changes:
- Don't generate a parent ID in Transaction.ParentID (name doesn't suggest that, and I don't think it's generally desirable behaviour anyway)
- Add parentID as an immutable field to the top-level Transaction and Span types. When creating a transaction or span, set these fields. Then we can access them even after they're ended, like we can for TraceContext.
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪Log outputExpand to view the last 100 lines of log output
|
Adds a new `parentID` field to both `Transaction` and `Span` structures. The `parentID` is only populated at start time and remains immutable throughout the lifecycle of the Transaction or Span, making it possible to retrieve the `parentID` for either after they have ended. However, the `ParentID()` call might return a zero value `SpanID` when a trace has no parent. Signed-off-by: Marc Lopez <marc5.12@outlook.com>
axw
reviewed
Apr 23, 2021
Removes the `TransactionData.parentSpan` field in favor of the new `Transaction.parentID` and updates `EnsureParent()` to update that field and `Transaction.ParentID()` to hold a read lock to read `tx.parentID`. Signed-off-by: Marc Lopez <marc5.12@outlook.com>
axw
approved these changes
Apr 23, 2021
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
|
Still TODO is entry in the change log, once the upcoming version is released, we should update this PR with the change log entry. |
Turns out I misunderstood the agreed upon definition of feature freeze for agent development. @marclop it's OK to merge this now (with changelog updated). |
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
|
Windows failed due to CI platform issues. Merging, nothing platform-specific here. |
stuartnelson3
pushed a commit
that referenced
this pull request
Jul 30, 2021
* apm: add ParentID methods to Span and Transaction Adds two new methods to the `Span` and `Transaction` types, expanding the API so consumers can extract the ParentIDs should they desire to. The `Transaction.ParentID()` call reuses the already existing method `Transaction.EnsureParent()` since it handles all the cases already. `Span.ParentID()` is much simpler and does not generate a new SpanID when `parentID` is zero or nil, simply returns an empty `SpanID` with its zero value. Closes #909. Signed-off-by: Marc Lopez <marc5.12@outlook.com> * Address review comments Adds a new `parentID` field to both `Transaction` and `Span` structures. The `parentID` is only populated at start time and remains immutable throughout the lifecycle of the Transaction or Span, making it possible to retrieve the `parentID` for either after they have ended. However, the `ParentID()` call might return a zero value `SpanID` when a trace has no parent. Signed-off-by: Marc Lopez <marc5.12@outlook.com> * Address review comments Removes the `TransactionData.parentSpan` field in favor of the new `Transaction.parentID` and updates `EnsureParent()` to update that field and `Transaction.ParentID()` to hold a read lock to read `tx.parentID`. Signed-off-by: Marc Lopez <marc5.12@outlook.com> * Remove SpanData.parentID in favor of Span.parentID Signed-off-by: Marc Lopez <marc5.12@outlook.com> * Update changelog Signed-off-by: Marc Lopez <marc5.12@outlook.com>
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.
Description
Adds a new
parentIDfield to bothTransactionandSpanstructures.The
parentIDis only populated at start time and remains immutablethroughout the lifecycle of the Transaction or Span, making it possible
to retrieve the
parentIDfor either after they have ended.However, the
ParentID()call might return a zero valueSpanIDwhena trace has no parent.
Removes the
TransactionData.parentSpanfield in favor of the newTransaction.parentIDand updatesEnsureParent()to update that fieldand
Transaction.ParentID()to hold a read lock to readtx.parentID.Last, removes the
SpanData.parentIDfield and instead relies on thenew top level
Span.parentID.Related issues
Closes #909.