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

Trace SDK: introduce span limits support #719

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

bio-aeon
Copy link
Contributor

@bio-aeon bio-aeon commented Aug 7, 2024

Resolves #481

It's a kind of pre-PR. If anyone thinks that something could be better done differently, let's discuss.
Also need to agree on how exactly to apply OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT - should it be some method of Attributes trait or something else.

@bio-aeon bio-aeon force-pushed the trace-sdk/support-span-limits branch 2 times, most recently from 02393d8 to 19ed7f4 Compare August 7, 2024 02:17
@iRevive
Copy link
Contributor

iRevive commented Aug 7, 2024

Hey, thanks for the contribution! The concept looks promising. I will take a thorough review this week.

@bio-aeon bio-aeon force-pushed the trace-sdk/support-span-limits branch 2 times, most recently from 93fed39 to 54b9a13 Compare August 7, 2024 18:32
@iRevive iRevive added tracing Improvements to tracing module sdk module Features and improvements to the sdk module labels Aug 11, 2024
@iRevive
Copy link
Contributor

iRevive commented Aug 13, 2024

Also need to agree on how exactly to apply OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT - should it be some method of Attributes trait or something else.

Good question. I don't see a simple way to make Attributes respect the value limit.

We can truncation attribute values in the SdkSpanBackend and SdkSpanBuilder:

SdkSpanBackend#addAttributes
SdkSpanBackend#addEvent
SdkSpanBackend#addEvent

SdkSpanBuilder#addAttribute
SdkSpanBuilder#addAttributes
SdkSpanBuilder#addLink

Or, we can squeeze the truncation logic in the specialized implementation of the LimitedData:

object LimitedData {
  def attributes(sizeLimit: Int, valueLengthLimit: Int): LimitedData[Attribute[_], Attributes] = 
    AttributesLimitedData(sizeLimit, valueLengthLimit)
    
  private final case class AttributesLimitedData(...) extends LimitedData[Attribute[_], Attributes] {
    def append(a: Attribute[_]): LimitedData[Attribute[_], Attributes] = 
      if (elements.size < limit) {
        copy(elements = elements.append(limitValueLength(a)))
      } else {
        copy(dropped = dropped + 1)
      }
      
    private def limitValueLength(a: Attribute[_]) = 
      a.key.`type` match {
        case AttributeType.String =>
          Attribute(a.key.name, value.asInstanceOf[String].take(valueLengthLimit))
        case AttributeType.StringSeq =>
          Attribute(a.key.name, value.asInstanceOf[Seq[String]].take(valueLengthLimit))
        case AttributeType.BooleanSeq =>
          ...
        case _ => 
          a
     }
  }
}

@iRevive
Copy link
Contributor

iRevive commented Aug 13, 2024

We should also use SpanLimitsAutoConfigure in the TracerProviderAutoConfigure:

for {
  ...
  processors <- configureProcessors(config, exporters)
+ spanLimits <- SpanLimitsAutoConfigure[F].configure(config)
  
  tracerProviderBuilder = {
    val builder = SdkTracerProvider
      .builder[F]
      ...
+     .withSpanLimits(spanLimits)

    processors.foldLeft(builder)(_.addSpanProcessor(_))
  }
  
  ...
} yield tracerProvider

That way, the TracerProvider will use limits configured via system properties or env variables (or the default ones).

@bio-aeon bio-aeon force-pushed the trace-sdk/support-span-limits branch from 54b9a13 to c7730be Compare August 15, 2024 16:04
@bio-aeon
Copy link
Contributor Author

Or, we can squeeze the truncation logic in the specialized implementation of the LimitedData

On the one hand, yes, it would be nice to avoid calling truncation method in multiple places. On the other hand, the logic of LimitedData methods is now generalized using Semigroup, and it would be nice to find a way not to rewrite these methods completely only for Attributes (you can argue at this point). I'll try to think this week a bit.

We should also use SpanLimitsAutoConfigure in the TracerProviderAutoConfigure

Done.

@NthPortal
Copy link
Contributor

my only concern here is that, because Attributes is unordered, this will end up dropping required attributes in favour of optional attributes

@bio-aeon bio-aeon force-pushed the trace-sdk/support-span-limits branch from c7730be to feaa1f1 Compare August 25, 2024 00:23
@bio-aeon bio-aeon force-pushed the trace-sdk/support-span-limits branch from feaa1f1 to 3a88831 Compare August 25, 2024 00:34
@bio-aeon
Copy link
Contributor Author

bio-aeon commented Aug 25, 2024

@iRevive I have added attribute values truncation inside of LimitedData similar to what you mentioned above, but keeping generalized LimitedData methods implementation. If this is ok to you, then it seems the only thing left to decide is what to do with attributes sorting.

@iRevive
Copy link
Contributor

iRevive commented Aug 26, 2024

my only concern here is that, because Attributes is unordered, this will end up dropping required attributes in favour of optional attributes

It's a valid concern indeed. I will address this in the follow-up PR.

Copy link
Contributor

@iRevive iRevive left a comment

Choose a reason for hiding this comment

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

Looks good.
I will address attribute ordering in the follow-up PR.

@iRevive
Copy link
Contributor

iRevive commented Aug 26, 2024

Hm, the specification doesn't mandate that the order must be preserved.

Here is a breakdown per implementation:

  1. Javascript - Attributes is a key-value object. Order is preserved
  2. Python - Attributes is a dictionary. Order is preserved
  3. Java - ArrayBasedAttributes - a user-facing implementation, i.e. when you use JAttributes.empty or JAttributes.builder - attributes are sorted by key. AttributesMap - attributes are stored in the HashMap, the order may vary.

This weekend, I've experimented with different structures: Vector, ListMap, VectorMap, etc.
Generally, other structures tend to be slower than the Map.

I would keep things as-is for now.

@NthPortal
Copy link
Contributor

AttributesMap is also not spec-compliant, because it's mutable (and possibly other reasons as well? I don't recall)

@iRevive
Copy link
Contributor

iRevive commented Aug 27, 2024

AttributesMap is also not spec-compliant, because it's mutable (and possibly other reasons as well? I don't recall)

Yep. I see that Otel java uses AttributesMap in the SdkSpan and SdkSpanBuilder.
Since you cannot access current attributes (e.g. span.getAttributes and builder.getAttributes) they are fine with that.

And user-facing implementation (ArrayBasedAttributes) is immutable.

@iRevive iRevive merged commit 8b235bc into typelevel:main Aug 27, 2024
10 checks passed
@bio-aeon bio-aeon deleted the trace-sdk/support-span-limits branch August 27, 2024 11:05
mzuehlke added a commit to mzuehlke/http4s-otel4s-middleware that referenced this pull request Sep 3, 2024
Adapt for breaking changes due to the introduction of span limits.
See: typelevel/otel4s#719

closes: http4s#122
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sdk module Features and improvements to the sdk module tracing Improvements to tracing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trace SDK: support span limits
3 participants