Feat: Add Response Modifier Option

[riandyrn]

This PR offers a new option called WithResponseModifier, which allows end-users to modify the middleware's response.

So in the scenario mentioned in #56, the end user could create their own modifier just like demonstrated here.

In addition to adding the WithResponseModifier option, I also created a predefined modifier called ResponseModifierTraceIDResponseHeader. It allows the end user to show the trace ID in the response header along with its sampled status, catering to the use case I mentioned here.

[riandyrn]

Hello, @costela

Let me know your feedback about this PR.

[ilhamsyahids]

We need to also add test for testing this behaviour

[riandyrn]

Ok, I've added the test for this, @ilhamsyahids.

However, during the process, I realized it is better to verify the HeaderKeyFunc() during initialization since the end user will immediately know something is wrong (not waiting until there is an incoming request, just like now).

You can check the commit 0fb3a5e for details.

[ilhamsyahids]

Since we move to initialization, I think we can move headerKey as well like this:

func ResponseModifierTraceIDResponseHeader(opt ResponseModifierTraceIDResponseHeaderOption) ResponseModifier {
	if err := opt.Validate(); err != nil {
		panic(err)
	}

	// moved headerKey here
	headerKey := DefaultTraceResponseHeaderKey
	if opt.HeaderKeyFunc != nil {
		headerKey = opt.HeaderKeyFunc()
	}

	return func(ctx context.Context, w http.ResponseWriter) {
		// remove headerKey

		...
	}
}

[riandyrn]

Haha.., you are right, @ilhamsyahids! 😁

Ok, updated in 10e1b73. Please recheck.

[ilhamsyahids]

Hello @riandyrn, everything is looking good except for above minor request

[ilhamsyahids]

LGTM 🔥👍

Nice idea to handle different use cases!

[costela]

I'm a bit concerned this may be too powerful a construct. Suddenly my middleware has multiple middlewares itself?

And, slightly confusingly, these "response modifiers" are actually called before my code responds?

Don't get me wrong: I think I understand the reasoning, and it would solve my problem, so I don't want to come across too harsh 😅, but it just feels a bit like shooting a fly with a missile.

[riandyrn]

Hello, @costela

Suddenly my middleware has multiple middlewares itself?

Haha.., valid point. Sometimes I forget the end user can just write their own middleware. So essentially ResponseModifier is not necessary. 😂

Okay, if we return to the root problem, essentially, we want to inform the end user whether the trace is sampled or not. Otherwise, the end user will be misled into thinking the trace is always sampled.

However, at the same time, we want to also minimize unexpected changes on the user side.

So what do you think if we just do the following:

1. Deprecate WithTraceIDResponseHeader(headerKeyFunc func() string) Option.
2. Supersede it with WithTraceIDResponseHeaderConfigurable(cfg WithTraceIDResponseHeaderConfig) Option.
3. We define HeaderKeyFunc(): string & IncludeSampledStatus: bool inside WithTraceIDResponseHeaderConfig.

By doing this we can achieve our main goal while minimizing the impact on the user side.

What do you think, @costela? Or maybe you have another perspective?

cc: @ilhamsyahids

[costela]

Yes, that sounds cool!

However, I do wonder if we can't siplify it even further? E.g.: what's the concrete use-case for the HeaderKeyFunc? Since it takes no parameter, it can't really use any context to derive the header name? In that case there seems to be little gain compared to a string? OTOH, if we do want more configurability, than maybe we also need the context in that function?

How about something like this?

func WithTraceResponseHeaders(opts TraceResponseHeadersConfig) Option { /* ... */ }

type TraceHeaderConfig struct {
  TraceIDHeader string // if non-empty overrides the default of X-Trace-ID
  TraceSampledHeader string // if non-empty overrides the default of X-Trace-Sampled
}

If the consumer doesn't want to expose some of the headers, I don't think we need an extra bool: the consumer can drop them from the response directly themselves.

So we keep the API simple, provide the one thing the consumer cannot easily do by themselves and have a relatively clean path forward in case we need more headers.

WDYT?

[riandyrn]

Nice, I like your suggestion, @costela! It is simple and hits all the marks.

Let us implement it your way. 👍

Would you like to create a new PR for it?

Also related to this:

func WithTraceResponseHeaders(opts TraceResponseHeadersConfig) Option { /* ... */ }

I think it's better to name the config cfg rather than opts.

[costela]

Sorry I didn't get around to it yet. I'll reopen my previous MR and implement a first pass there! 👍

[costela]

Unfortunately I could not reopen the original PR, so I opened #59

PTAL 🙏

[riandyrn]

Superseded by #59