Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

feat(SugaredTracer): add draft of sugaredTracer #173

Closed

Conversation

secustor
Copy link

Early draft of a SugaredTracer, intended as basis for discussions and implementation ideas.

cc @cartermp

Signed-off-by: secustor sebastian@poxhofer.at

Signed-off-by: secustor <sebastian@poxhofer.at>
Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Overall I like the interface. Really my only question is if withSpan (where it's not marked a active) should exist as an option.

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

I like the idea of creating a new wrapping interface that does not require SDK to provide any extra interface implementations!

try {
const ret = fn(span);
// if fn is an async function attach a recordException and spanEnd callback to the promise
if (ret instanceof Promise) {
Copy link
Member

Choose a reason for hiding this comment

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

We don't generally brand check Promise in JavaScript. Typically, a duck-type checking like typeof ret.then === 'function' is preferred for thanables.

return new SugaredTracer(tracer)
}

class SugaredTracer implements Tracer {
Copy link
Member

Choose a reason for hiding this comment

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

Bikeshedding: I'm not sure if this name is appropriate or comprehensible to users. But I didn't come up with a better name :(

Copy link
Author

Choose a reason for hiding this comment

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

I have based the current iteration on the discussion in this issue, but I'm open for alternatives :)

Copy link
Contributor

Choose a reason for hiding this comment

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

How about ConvenientSpanTracer or AutoClosingSpanTracer?

Copy link
Author

Choose a reason for hiding this comment

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

AutoClosingSpanTracer: IMO this will not include only this feature, but could be expanded in the future. Therefore I think we should not use something specific like this.
ConvenientSpanTracer: Is fitting but, Sugared seems to be widely used for such scenarios.

@dyladan
Copy link
Member

dyladan commented Oct 10, 2022

@secustor this package is being moved into the main repo. Can you please re-apply this change there? I realize it isn't ideal, but it should at least get more attention there.

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

Successfully merging this pull request may close these issues.

None yet

4 participants