Skip to content

feat: Add convenience helper functions for apm #2556

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

Closed
wants to merge 1 commit into from

Conversation

HazAT
Copy link
Member

@HazAT HazAT commented Apr 24, 2020

I want to open a discussion if we want to support such API.

This PR introduced two helper functions withTransaction & withSpan
and a helper function on the Span to create a child with a callback span.withChild.
Functionally can be found in the test cases.

Here is just a brief example of what would be possible with these helpers:
Given:

async function testRequest(): PromiseLike<void> {
  return;
}
async function testHelpers() {
  return withTransaction('myRequest', { op: 'testHelpers' }, async (transaction: Span) =>
    transaction.withChild({ description: 'request' }, async () => testRequest()),
  );
}

vs. without helpers

async function testHelpers() {
  const transaction = getCurrentHub().startSpan({
    op: 'testHelpers',
    transaction: 'myRequest'
  });

  const childSpan = transaction.child({ description: 'request' });
  await testRequest();
  childSpan.finish();

  transaction.finish();
}

It mostly helps people not to forget to call finish().
The idea spawned from a random conversation in Slack.

@HazAT HazAT self-assigned this Apr 24, 2020
@HazAT HazAT force-pushed the hazat/apm-helper-experiment branch from 97e1d50 to 46a5abb Compare April 24, 2020 11:46
@getsentry-bot
Copy link
Contributor

getsentry-bot commented Apr 24, 2020

Warnings
⚠️ Please add a changelog entry for your changes.
Messages
📖

@sentry/browser bundle gzip'ed minified size: (ES6: 15.9941 kB) (ES5: 16.9521 kB)

📖 ✅ TSLint passed

Generated by 🚫 dangerJS against 46a5abb

@HazAT
Copy link
Member Author

HazAT commented Apr 24, 2020

cc @evanpurkhiser @dashed

@HazAT HazAT requested a review from a team April 24, 2020 11:51
Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

I'm in favor of something in this direction, just dislike the span x transaction. It means "library" code needs to stick with span, and somewhere in the top level users need to create a transaction; feels like extra cognitive burden.

}

/**
* Create a span from a callback. Make sure you wrap you `withSpan` calls into a transaction.
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a draft of this idea but without the caveat of "make sure you wrap with a transaction".

Generally this is one of the reasons I don't like the transaction x span distinction, because it makes it harder to arbitrarily wrap code.

@HazAT
Copy link
Member Author

HazAT commented Apr 24, 2020

I'm in favor of something in this direction, just dislike the span x transaction. It means "library" code needs to stick with span, and somewhere in the top level users need to create a transaction; feels like extra cognitive burden.

True, I also don't like it, the problem is we can't get rid of a Transaction and the users need to understand the concept because it has more semantic behind it.
I am afraid this is here to stay.

@dashed
Copy link
Member

dashed commented Apr 24, 2020

I like it 👍

@HazAT
Copy link
Member Author

HazAT commented May 25, 2020

I am going to close this PR since this one #2600 is the successor
Let's wait for adding more API like this until we verified that the baseline of what the PR provides is enough.

@HazAT HazAT closed this May 25, 2020
@HazAT HazAT deleted the hazat/apm-helper-experiment branch May 25, 2020 11:37
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.

4 participants