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

Remove effect constraint from TextMapPropagator #332

Merged
merged 8 commits into from
Nov 1, 2023

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Oct 5, 2023

Motivation

While working on #325, it turned out that the effectful injection into the mutable carrier was optional.
The majority of the typelevel ecosystem revolves around immutable data structures.

So, is there any actual use case for the effectful injection into the mutable carrier?

The changes

  • Move propagation-specific functionality to the context.propagation package
  • TextMapPropagator - add def fields: List[String]
  • TextMapPropagator - remove def inject[A: TextMapUpdater](...): F[Unit]
  • TextMapPropagator - rename def injected[A: TextMapUpdater](...): A to inject
  • TextMapPropagator and ContextPropagators - remove effect F constraint
  • Add human-friendly toString
  • Add tests

What does the spec say?

https://opentelemetry.io/docs/specs/otel/context/api-propagators/#inject

Note

Injects the value into a carrier. For example, into the headers of an HTTP request.

Required arguments:

  • A Context. The Propagator MUST retrieve the appropriate value from the Context first, such as SpanContext, Baggage or another cross-cutting concern context.
  • The carrier that holds the propagation fields. For example, an outgoing message or HTTP request.

The new implementation complies with this requirement.


https://opentelemetry.io/docs/specs/otel/context/api-propagators/#textmap-inject

Warning

Injects the value into a carrier. The required arguments are the same as defined by the base Inject operation.

Optional arguments:

  • A Setter to set a propagation key/value pair. Propagators MAY invoke it multiple times in order to set multiple pairs. This is an additional argument that languages are free to define to help inject data into the carrier.

Since we use TextMapUpdater rather than TextMapSetter, in some sense, we violate the requirement.

@iRevive iRevive force-pushed the propagator-drop-effect branch from 68f5fb7 to 5691fa3 Compare October 5, 2023 16:22
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.

left a few comments, but nothing blocking. overall LGTM!

@NthPortal
Copy link
Contributor

NthPortal commented Oct 5, 2023

Since we use TextMapUpdater rather than TextMapSetter, in some sense, we violate the requirement.

if we rename TextMapUpdater to TextMapImmutableSetter, does that make us compliant? 😅

@iRevive
Copy link
Contributor Author

iRevive commented Oct 6, 2023

I ended up renaming create to of.
What if we keep apply for simple constructors and of - for the constructors with varargs where we do some 'magic' under the hood (e.g. flattening out Multi instances)?

@iRevive iRevive added this to the 0.4 milestone Oct 11, 2023
@iRevive iRevive modified the milestones: 0.4, 0.3 Oct 28, 2023
@NthPortal
Copy link
Contributor

is there anything else this needs before merging?

@iRevive
Copy link
Contributor Author

iRevive commented Nov 1, 2023

Nope, I can merge it right away.

@iRevive iRevive merged commit dfb9724 into typelevel:main Nov 1, 2023
9 checks passed
@iRevive iRevive deleted the propagator-drop-effect branch November 1, 2023 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants