fix(traceloop-sdk): omit spans of non-traceloop instrumentations by default when using standalone processor

[galkleinman]

• Add tests (will do it in a different PR, i'm having hard times testing it and want to release this one for a customer)

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to a03bf30 in 2 minutes and 42 seconds. Click for details.

• Reviewed 174 lines of code in 4 files
• Skipped 0 files when reviewing.
• Skipped posting 6 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. packages/sample-app/src/sample_otel_sdk.ts:16

• Draft comment:
  Ensure that the 'allowedInstrumentationLibraries' value ('my-sample-app') is a valid instrumentation library identifier. This setting filters spans to only your app, which might be expected but double-check if it's the intended behavior.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50% None

2. packages/traceloop-sdk/src/lib/tracing/index.ts:284

• Draft comment:
  Consistently using ATTR_SERVICE_NAME improves clarity. Ensure that the 'appName' fallbacks align with expectations.
• Reason this comment was not posted:
  Confidence changes required: 10% <= threshold 50% None

3. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:100

• Draft comment:
  Avoid mutating the shared 'traceloopInstrumentationLibraries' array. Consider cloning it (e.g., using spread syntax) before pushing additional libraries.
• Reason this comment was not posted:
  Marked as duplicate.

4. packages/traceloop-sdk/src/lib/tracing/tracing.ts:4

• Draft comment:
  Renaming the TRACER_NAME to '@traceloop/node-server-sdk' aligns with naming conventions. Verify that this change is propagated to consumers.
• Reason this comment was not posted:
  Confidence changes required: 10% <= threshold 50% None

5. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:12

• Draft comment:
  There's a potential typographical error: 'ASSOCATION_PROPERTIES_KEY' might be intended to be spelled 'ASSOCIATION_PROPERTIES_KEY'. Please double-check if this is an intentional naming or a typo.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. packages/traceloop-sdk/src/lib/tracing/tracing.ts:7

• Draft comment:
  Typographical error: The constant 'ASSOCATION_PROPERTIES_KEY' appears to be misspelled. Consider renaming it to 'ASSOCIATION_PROPERTIES_KEY' to ensure consistency and clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_tMdN6lL2Ci2s8ZV6

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[nirga]

I'd have a slightly different behavior here - if the parameter is undefined - then it's all instrumentation libraries. If it's set - then whatever is set (which can either be an array of library names or TRACELOOP_INSTRUMENTATION_LIBRARIES) - wdyt?

[galkleinman]

I thought of this option, it'll force users of the standalone processor to assign TRACELOOP_INSTRUMENTATION_LIBRARIES for instrumentationLis argument instead of having it working by default

[galkleinman]

it makes more sense tho imo, but has some more friction