Skip to content
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

feat: Make tracing object fluent #1381

Closed
wants to merge 2 commits into from
Closed

feat: Make tracing object fluent #1381

wants to merge 2 commits into from

Conversation

stayallive
Copy link
Collaborator

This could make writing tracing code easier and more readable depending on your preference.

The upside is that this unlocks a fluent way of writing. I don't think there are any actual downsides (apart from BC).

I believe this is a backward incompatible change, especially Span is not a final class so I don't think we can make this change in 3.x but maybe this is a good start for 4.x and allows us to make the tracing code simpeler/shorter to use. Possibly we could already convert the final classes but not a 100% sure if that is also a BC.

If we think this is acceptable and a good change I would propose that going forward (possibly starting with 4.x) to make all future APIs fluent where it makes sense.


A small example partly taken from the laravel codebase:

Before:

$context = TransactionContext::fromHeaders($sentryTraceHeader, $baggageHeader);
$context->setOp('http.server');
$context->setData([
    'url' => '/' . ltrim($request->path(), '/'),
    'method' => strtoupper($request->method()),
]);
$context->setStartTimestamp($requestStartTime);

$bootstrapSpan = $this->addAppBootstrapSpan($request);

$appContextStart = new SpanContext();
$appContextStart->setOp('laravel.handle');
$appContextStart->setStartTimestamp($bootstrapSpan ? $bootstrapSpan->getEndTimestamp() : microtime(true));

$this->transaction->startChild($appContextStart);

After:

$context = TransactionContext::fromHeaders($sentryTraceHeader, $baggageHeader)
    ->setStartTimestamp($requestStartTime)
    ->setOp('http.server')
    ->setData([
        'url' => '/' . ltrim($request->path(), '/'),
        'method' => strtoupper($request->method()),
    ]);

$bootstrapSpan = $this->addAppBootstrapSpan($request);

$appContextStart = (new SpanContext)
    ->setStartTimestamp($bootstrapSpan ? $bootstrapSpan->getEndTimestamp() : microtime(true))
    ->setOp('laravel.handle');

Base automatically changed from dsc to develop September 28, 2022 09:25
@stayallive stayallive changed the title Make tracing object fluent feat: Make tracing object fluent Sep 28, 2022
@cleptric
Copy link
Member

I really like how this would improve our API.

On the other hand, we will force people to update all their instrumentation code when they upgrade to 4.0.
And I have the feeling that this work is not really worth the gain for them 🙁

@stayallive
Copy link
Collaborator Author

Why would they? The old code still works with this change. In my description, both code blocks work perfectly fine after this change 😄

The only thing is that people extending our classes (read: Span) need to add return $this;. But I'm not sure why people would extend our Span class in the first place so unsure this is a real problem.

@cleptric
Copy link
Member

cleptric commented Sep 29, 2022

Ok, then let's mark Span as final. No, we can't...

I would like to have a discussion about this in a different issue, where we could discuss annotating stuff as @internal vs making everything final and impossible for people to extend (there might be uses-cases for that).

Base automatically changed from develop to master October 5, 2022 09:30
@cleptric
Copy link
Member

Closing for now but keeping the branch for later 🙂

@cleptric cleptric closed this May 15, 2023
@cleptric cleptric reopened this Sep 12, 2023
@cleptric cleptric changed the base branch from master to 4.x September 12, 2023 13:12
@getsantry getsantry bot added the Stale label Oct 18, 2023
@cleptric cleptric removed the Stale label Oct 18, 2023
@getsentry getsentry deleted a comment from getsantry bot Oct 18, 2023
@stayallive
Copy link
Collaborator Author

Closing this in favor of #1601.

@stayallive stayallive closed this Oct 24, 2023
@cleptric cleptric deleted the fluent-tracing branch November 6, 2023 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants