-
Notifications
You must be signed in to change notification settings - Fork 36
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
Trace SDK: introduce span limits support #719
Conversation
02393d8
to
19ed7f4
Compare
sdk/trace/src/main/scala/org/typelevel/otel4s/sdk/trace/SpanLimits.scala
Outdated
Show resolved
Hide resolved
Hey, thanks for the contribution! The concept looks promising. I will take a thorough review this week. |
93fed39
to
54b9a13
Compare
sdk/trace/src/main/scala/org/typelevel/otel4s/sdk/trace/data/EventData.scala
Outdated
Show resolved
Hide resolved
Good question. I don't see a simple way to make We can truncation attribute values in the
Or, we can squeeze the truncation logic in the specialized implementation of the 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
}
}
} |
We should also use 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). |
54b9a13
to
c7730be
Compare
On the one hand, yes, it would be nice to avoid calling truncation method in multiple places. On the other hand, the logic of
Done. |
my only concern here is that, because |
c7730be
to
feaa1f1
Compare
feaa1f1
to
3a88831
Compare
@iRevive I have added attribute values truncation inside of |
It's a valid concern indeed. I will address this in the follow-up PR. |
There was a problem hiding this 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.
Hm, the specification doesn't mandate that the order must be preserved. Here is a breakdown per implementation:
This weekend, I've experimented with different structures: I would keep things as-is for now. |
|
Yep. I see that Otel java uses And user-facing implementation (ArrayBasedAttributes) is immutable. |
Adapt for breaking changes due to the introduction of span limits. See: typelevel/otel4s#719 closes: http4s#122
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 ofAttributes
trait or something else.