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

Allow fluent use of Transaction::setName() method #1687

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

mfb
Copy link
Contributor

@mfb mfb commented Jan 19, 2024

For better developer experience, I'd say Transaction::setName() should also be a fluent method

@cleptric
Copy link
Member

Any use case in mind? You normally set the name via the context.

@mfb
Copy link
Contributor Author

mfb commented Jan 19, 2024

I set the name via the context, and then later in the request life cycle, set it to the route name; Sentry Laravel SDK also calls Transaction::setName() in the same way.

@cleptric
Copy link
Member

Yep, but do you have any code example in mind where the fluent API matters?

@mfb
Copy link
Contributor Author

mfb commented Jan 19, 2024

I don't think the fluent API matters per se, it's just to make the methods consistent for better developer experience.

The other set* methods (inherited from the Span class) return $this, so a developer might reasonably expect this method to also return $this (until they execute the code or run phpstan and notice it doesn't work of course :)

@mfb
Copy link
Contributor Author

mfb commented Jan 19, 2024

The goal would be for something like this to work

$transaction->setTags(['bar' => 'baz'])
  ->setName('foo')
  ->finish();

Currently something like this is required:

$transaction->setTags(['bar' => 'baz'])
  ->setName('foo');
$transaction->finish();

@cleptric
Copy link
Member

cleptric commented Jan 19, 2024

I think this won't work. setTags returns a Span and calling setName on it throws :/

@mfb
Copy link
Contributor Author

mfb commented Jan 19, 2024

I think this won't work. setTags returns a Span and calling setName on it throws :/

This code works fine in my testing; setTags() returns the Transaction and nothing was thrown. What is thrown for you?

@cleptric
Copy link
Member

hmm, then maybe I misremembered, we had a similar issue before. Anyways, a test please and this is good to go.

@stayallive
Copy link
Collaborator

We had a self return type hint on span methods which didn't work (because it returned the Span instead of Transaction). Should work fine in the transaction methods though 👍

@mfb
Copy link
Contributor Author

mfb commented Jan 19, 2024

I'm unclear what wouldn't work with return types on Span. If a Transaction method is supposed to return a Span, I thought it works fine because a Transaction is a Span.

But in any case yeah that issue isn't relevant for this issue.

@cleptric cleptric merged commit 617dde3 into getsentry:master Jan 22, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants