Skip to content

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Aug 13, 2025

Closes #1029

@mergify mergify bot added module:core Features and improvements to core module module:oteljava Features and improvements to the oteljava module module:sdk Features and improvements to the sdk module module:sdk:contrib:aws Feature and improvements to the sdk contrib aws module module:sdk:exporter Features and improvements to the sdk exporter module metrics Improvement to metrics module tracing Improvements to tracing module logs Improvements to logs module labels Aug 13, 2025
Copy link
Contributor

@NthPortal NthPortal left a comment

Choose a reason for hiding this comment

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

LGTM barring the PassThroughPropagator thing

Comment on lines -71 to +74
val callback = cb { (value, attributes) =>
observable.record(cast(value), attributes)
val measurement = new ObservableMeasurement.Unsealed[F, A] {
def record(value: A, attributes: Attributes): F[Unit] =
observable.record(cast(value), attributes)
Copy link
Contributor

Choose a reason for hiding this comment

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

that's an interesting side effect of this change

@iRevive iRevive force-pushed the feature/seal-public-traits branch from dfd01b5 to 3a41e63 Compare August 19, 2025 06:36
@iRevive iRevive changed the title [WIP] Seal public traits Seal public traits Aug 19, 2025
@iRevive iRevive marked this pull request as ready for review August 19, 2025 06:36
@mergify mergify bot added the module:semconv Something to do with semantic conventions label Aug 19, 2025
@iRevive
Copy link
Contributor Author

iRevive commented Aug 19, 2025

A few items remain 'unsealed':


wrt TextMap* - we can seal the interface and provide public apply. e.g.:

sealed trait TextMapGetter[A] {
  def get(carrier: A, key: String): Option[String]
  def keys(carrier: A): Iterable[String]
}

object TextMapGetter {
  def apply[A](g: (A, String) => Option[String], k: A => Iterable[String]): TextMapGetter[A] = ...
}

However, I'm unsure if it's worth the hassle, as it will affect third-party integration, which is a major inconvenience.

@iRevive iRevive force-pushed the feature/seal-public-traits branch from b16a800 to d42df76 Compare August 19, 2025 07:19
@NthPortal
Copy link
Contributor

I don't think we should seal the TextMap* instances. if we need to evolve their APIs to add a method, it would necessitate changing the factory methods anyway, so I don't think there is any benefit

@iRevive iRevive merged commit 92e18bf into typelevel:main Aug 20, 2025
11 checks passed
@iRevive iRevive deleted the feature/seal-public-traits branch August 20, 2025 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logs Improvements to logs module metrics Improvement to metrics module module:core Features and improvements to core module module:oteljava Features and improvements to the oteljava module module:sdk:contrib:aws Feature and improvements to the sdk contrib aws module module:sdk:exporter Features and improvements to the sdk exporter module module:sdk Features and improvements to the sdk module module:semconv Something to do with semantic conventions tracing Improvements to tracing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider prioritizing sealed interfaces
2 participants