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

Improve DX on Sentry.trace #8075

Closed
dcramer opened this issue May 8, 2023 · 6 comments · Fixed by getsentry/rfcs#101
Closed

Improve DX on Sentry.trace #8075

dcramer opened this issue May 8, 2023 · 6 comments · Fixed by getsentry/rfcs#101

Comments

@dcramer
Copy link
Member

dcramer commented May 8, 2023

Collection of concerns carrying over from Slack:

await trace(
  {
    op: "gcs.file.write-stream",
    name: "gcs.file.write-stream",
    description: newFilename,
  },
  async () => {
    const writeStream = file.createWriteStream();
    await pump(data.file, writeStream);
  }
);

It appears op is required (at least with debug=true the logs look funky), but name is actually required per the type defs. AFAICT they're the same thing, so why are they different?

Additionally we do not document the trace function, and its key to success with instrumentation. I would expect this to at least be a subset of the OTel equiv docs, and I would expect them to exist on the core instrumentation page.

Both of these issues might carry over to every SDK, I'm not sure.

@AbhiPrasad
Copy link
Member

Name is required because it maps to transaction.name/span.description, which is what we use for suspect spans, and for generally identifying spans via the performance views.

Op is used for breakdowns and high level analysis, for example for this view:

image

In OpenTelemetry high level aggregates, like identifying if something is a database or http span is powered by semantic understanding of arbitrary span attributes: https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/. If a span has the http.method attribute for example, it's considered as an http span.

The trace function was recently added, and not documented as we were working toward getting it to a stable state. It'll be public soon.

@dcramer
Copy link
Member Author

dcramer commented May 8, 2023

Can't name default to op? Like isnt op actually required? Name is just our transaction specific thing, that only matters when its a transaction? Basically how do we avoid this issue where a customer has to provide 2x the same annotation that mean the same thing? I'm trying to get this to be as simplea s otel's api which only requires the op name as the first param (and optional context, which is not a simple DX, as the second).

Alternatively if name == description, same thing, but afaik there are only two columns outside of tags/data

@AbhiPrasad
Copy link
Member

Actually name is required - it's the unique identifier that groups spans together, op is just a higher level bucket. For example a GET /items is much different than a POST /users, but they are both considered http operations.

For all intents and purposes transaction name === span description === otel span name.

OTEL doesn't have anything equivalent to how we define op, this is a Sentry specific thing.

I agree to 2x annotation is annoying, I want us to fix this and properly consolidate name/description.

@AbhiPrasad AbhiPrasad self-assigned this May 10, 2023
@AbhiPrasad
Copy link
Member

Copying over the initial proposal from notion: https://www.notion.so/sentry/Performance-simplified-59c8872f1173439b97386f14dd0ea0c6

Use Cases

User story: As a developer I can send performance data about my function calls to Sentry so I get duration information about those calls while maintaining correct parent child relationship between timing information.

Timing a function call:

SDKs should expose a decorator/wrapper function that automatically starts a span when the function starts, and stops the span when the function finishes.

  • this can be a decorator in python, class decorator in js, annotation in php, rust macro etc.
@Sentry.trace({ name: 'expensiveCalc' })
function expensiveCalc() {

}

// span that is created is provided to callback in case additional
// attributes have to be added.
// ideally callback can be async/sync
Sentry.trace({}, (_span) => expensiveCalc());

// If the SDK needs async/sync typed different we can expose this
// declare function Sentry.traceAsync(spanCtx, asyncCallback);

Under the hood, the SDK takes care of creating a transaction/span accordingly and managing if it should be on the scope.

Overriding when span is finished

In case the wrapped callback finish shouldn’t map to span finish, use a manual trace function to determine when exactly to finish the span.

function handler(req, res) {
  // todo name kind of sucks
  Sentry.traceManual({ name: 'expensiveCalc' }, (span) => {
		expensiveCalc();
    // span needs to finish on response end callback,
    // not when handler is finished.
    res.on('end', () => span.finish());
  })
}

Exact control over starting/stopping spans

In the case where you want even more control, manually call start span and finish span. Note that you’ll have to add this to the scope yourself if you do this (otherwise the callback based wrappers should be good enough)

// does not get put on scope, user's have to do this manually
const span = Sentry.startSpan({ name: 'expensiveCalc' });

expensiveCalc();

span.finish();

// Optionally SDKS can expose a Sentry.startActiveSpan API to have it
// be automatically put on the scope. This is dangerous because then the span
// is potenially never removed from the scope if users don't call finish on it.

// In Go and other langs with explicit ctx this probably needs to be
// Sentry.StartSpanFromContext(ctx, spanCtx)

Monkeypatching module functions

Inspired by functions_to_trace in the Python SDK, optionally allow for users to pass in a list of module/function paths that will be automatically wrapped with a function.

Sentry.init({
  functionsToTrace: {
    'moduleA.method1': {},
    // override span context
    'moduleA.method2': { name: 'bestModule' },
  }
  // alternate API is to use tuples
  // functionsToTrace: [
  //  ['moduleA.method1'],
  //  ['moduleA.method2', { name: 'bestModule' }]
  // ]
});

// We can also expose a wrapping method for languages which
// can't do the string based dynamic monkeypatching
const { fancyMethod as _fancyMethod } = require('third-party-library');
export const fancyMethod = Sentry.traceWrap(fancyMethod)

// We can also expose a method to automatically instrument all exported
// methods from a module
// 2nd argument is same as functionsToTrace
Sentry.traceModuleMethods('module-name', { 'method1': { name: 'renamed' } });

@Lms24
Copy link
Member

Lms24 commented May 15, 2023

Monkeypatching module functions

q: This would only work for CJS, right? Given that everyone is moving to esm (slooowly), should we even create this API?

@AbhiPrasad
Copy link
Member

q: This would only work for CJS, right? Given that everyone is moving to esm (slooowly), should we even create this API?

Other SDKs can take advantage of it, and JS would probably provide an esm specific solution with loaders.

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

Successfully merging a pull request may close this issue.

4 participants