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

SpanProcess.onStart does not have access to span attributes #4188

Closed
anuraaga opened this issue Oct 5, 2023 · 2 comments · Fixed by #4277
Closed

SpanProcess.onStart does not have access to span attributes #4188

anuraaga opened this issue Oct 5, 2023 · 2 comments · Fixed by #4277
Labels
bug Something isn't working priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization spec-noncompliant An existing feature incorrectly or incompletely implements the OTel spec. May or may not be a bug

Comments

@anuraaga
Copy link
Contributor

anuraaga commented Oct 5, 2023

What happened?

Steps to Reproduce

Access span.attributes in a SpanProcessor.onStart method

Expected Result

Read attributes

Actual Result

Attributes always empty

Additional Details

Span processor is called in span constructor

this._spanProcessor.onStart(this, context);

But attributes are set after

https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-sdk-trace-base/src/Tracer.ts#L150

Presumably it would be better to call the spanprocessor fromTracer.startSpan instead of constructor. Alternatively, attributes could be passed into the constructor though backwards compatibility may be challenging.

OpenTelemetry Setup Code

No response

package.json

No response

Relevant log output

No response

@anuraaga anuraaga added bug Something isn't working triage labels Oct 5, 2023
@pichlermarc pichlermarc added priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization spec-noncompliant An existing feature incorrectly or incompletely implements the OTel spec. May or may not be a bug and removed triage labels Oct 11, 2023
@ArtAhmetaj
Copy link
Contributor

So I checked the issue and want to be a contributor to this. I checked both your options on either calling the spanprocessor or modifying the constructor:

  • Calling the span processor
    Seems nice in hindsight but I fear that the span class is being used by others directly without it being used through the tracer (I am new to the project so I am assuming that based on also the @deprecated decorators), so if I push this functionality to the tracer that means that when using span directly the .onStart will not be triggered, unless I just expose this as a span method with the ability to override the on start hooks, if that functionality is fine then that would be okay.
  const span = new Span(
      this,
      context,
      name,
      spanContext,
      spanKind,
      parentSpanId,
      links,
      options.startTime
    );
    // Set initial span attributes. The attributes object may have been mutated
    // by the sampler, so we sanitize the merged attributes before setting them.
    const initAttributes = sanitizeAttributes(
      Object.assign(attributes, samplingResult.attributes)
    );
    span.setAttributes(initAttributes);
    span.registerProcessor(): 

Seems really clunky tho...

  • Adding the data in the constructor
    A simple solution when I had a look first on it:
constructor(
    parentTracer: Tracer,
    context: Context,
    spanName: string,
    spanContext: SpanContext,
    kind: SpanKind,
    parentSpanId?: string,
    links: Link[] = [],
    startTime?: TimeInput,
    _deprecatedClock?: unknown, // keeping this argument even though it is unused to ensure backwards compatibility,
    initAttributes?: Attributes
  ) {
    this.name = spanName;
    this._spanContext = spanContext;
    this.parentSpanId = parentSpanId;
    this.kind = kind;
    this.links = links;
    if(initAttributes){
      this.setAttributes(initAttributes);
    }

    const now = Date.now();
    this._performanceStartTime = otperformance.now();
    this._performanceOffset =
      now - (this._performanceStartTime + getTimeOrigin());
    this._startTimeProvided = startTime != null;

    this.startTime = this._getTime(startTime ?? now);

    this.resource = parentTracer.resource;
    this.instrumentationLibrary = parentTracer.instrumentationLibrary;
    this._spanLimits = parentTracer.getSpanLimits();
    this._spanProcessor = parentTracer.getActiveSpanProcessor();
    this._spanProcessor.onStart(this, context);
    this._attributeValueLengthLimit =
      this._spanLimits.attributeValueLengthLimit || 0;
  }

Doesn't seem like the span has optional variables like this till now atleast, but I think it would be simple enough, would not break in the constructor or the wrapped call in the constructor.

Let me know what you think and I can make a PR about it.

@anuraaga
Copy link
Contributor Author

Thanks for the clarification @ArtAhmetaj - indeed, I was thinking the constructor isn't called but you are right that it may be since it is currently a public API. Actually coincidentally, I found it used in such a way in Prisma instrumentation just yesterday. So the second approach seems better to me too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization spec-noncompliant An existing feature incorrectly or incompletely implements the OTel spec. May or may not be a bug
Projects
None yet
3 participants