Skip to content

Conversation

@DaveCTurner
Copy link
Contributor

Today the APM Tracer interface identifies each span by a raw string, but in practice there is structure to these strings: task-related spans have IDs like task-NNNN and spans that relate to REST requests have IDs like rest-NNNN. This convention is distributed across the codebase a little too widely, so with this commit we centralise it into a SpanId class, and introduce specific overrides for Task and RestRequest to avoid callers needing to construct IDs themselves.

Today the APM `Tracer` interface identifies each span by a raw string,
but in practice there is structure to these strings: task-related spans
have IDs like `task-NNNN` and spans that relate to REST requests have
IDs like `rest-NNNN`. This convention is distributed across the codebase
a little too widely, so with this commit we centralise it into a
`SpanId` class, and introduce specific overrides for `Task` and
`RestRequest` to avoid callers needing to construct IDs themselves.
@DaveCTurner DaveCTurner added >non-issue :Core/Infra/Core Core issues without another label v8.8.0 labels Mar 29, 2023
@DaveCTurner DaveCTurner requested a review from pugnascotia March 29, 2023 12:07
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Mar 29, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@DaveCTurner DaveCTurner removed the request for review from pugnascotia March 29, 2023 12:37
@DaveCTurner
Copy link
Contributor Author

@elasticmachine please run elasticsearch-ci/packaging-tests-windows-sample

@pgomulka pgomulka self-requested a review March 29, 2023 17:18
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM. I like this (potential) increase in future consistency.

Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

LGTM

@DaveCTurner DaveCTurner merged commit 431a7b2 into elastic:main Mar 30, 2023
@DaveCTurner DaveCTurner deleted the 2023-03-29-apm-tracer-destringify branch March 30, 2023 09:19
@DaveCTurner
Copy link
Contributor Author

Thanks both!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants