Expose codec level stream ID to Envoy::Http::Stream and Envoy::StreamInfo::StreamIdProvider

[reubent-goog]

Commit Message: Currently, there is no way to access the codec level stream id from either of these classes and we need it as part of some future work. Each codec implements its own protocolStreamId. Http/1 always returns nullopt and HTTP/2 + HTTP/3 return the stream id the codec level stream operates on. Rather than add a new field to Envoy::StreamInfo to expose this stream ID, we use the existing StreamIdProvider which has historically provided non-codec level IDs for logging.
Additional Description:
Risk Level: Low
Testing: Unit tests
Docs Changes: Yes
Release Notes: Yes
Platform Specific Features: No

[reubent-goog]

/assign @tyxia

[reubent-goog]

/assign @abeyad

[abeyad]

thanks Reuben! LGTM, just a couple minor nits/questions

[abeyad]

why is this implementation returning nullopt? where would this be invoked from?

[reubent-goog]

Good question. In OSS, this StreamIdProvider implementation is used for TCP and UDP proxy sessions which do not have codec level stream ids. In G3 it's used for another thing that also doesn't have a codec level stream id.

[tyxia]

Given that getCodecStreamId is only implemented and used in HttpStreamIdProviderImpl, can this function change to be a local member function of HttpStreamIdProviderImpl class?

[reubent-goog]

We could do that but then callers who only have a StreamInfo would have to check if their instance of StreamIdProvider is an HttpStreamIdProviderImpl and then cast it appropriately. This is error-prone and defeats the purpose of the interface only to save a few no-op implementations.

That said, I don't feel strongly about this. @abeyad, do you have an opinion?

[abeyad]

I think keeping as an interface method is better. Would rather not have casting if we are passing around a StreamIdProvider in the code.

[tyxia]

My point is if this function is only being used by one particular inherited class and remains unused by all other subclasses, then it is probably not a good candidate for interface function. Because it defeats the principle of Object-Oriented design : Pollution of the Base Class, Defeating Polymorphism and Liskov Substitution Principle (LSP)

dynamic_cast is one option that is designed for safe downcasting. Alternatively, we can expose stream_id to class StreamInfo directly rather than StreamIdProvider (like draft here #41913), we can avoid all those no-ops.

I don't have strong opinion if you both think current approach is a good, but i want to be a bit cautious here as it touches Envoy core code/interface

[reubent-goog]

remains unused by all other subclasses / all those no-ops.

There are only two implementations of StreamIdProvider: HttpStreamIdProvider and StreamIdProviderImpl so "all the other subclasses" is literally one class.

Pollution of the Base Class / one particular inherited class

My OO experience is in Java but when I hear "base class" I think of a class that has implementation in it maybe with some functions that are either overridden by or implemented in the concrete subclasses. StreamIdProvider does not have implementation in it, it only has pure virtual functions so it's an interface rather than a base class. That interface has only two implementations and I don't think adding a single function to an interface with two functions could really be considered "pollution".

Defeating Polymorphism

This isn't a well known anti-pattern so I don't know exactly what you mean by this but, in my experience, depending on implementation details is actually what defeats polymorphism (i.e. requiring callers to check if their StreamIdProvider instance is an HttpStreamIdProvider, casting and calling a member function unique to an implementation).

Liskov Substitution Principle (LSP)

Most of the time when I've heard LSP brought up as an argument against a polymorphic function it's when there is a true base class with implementation and one of methods overriden by the subclass causes a base class method to become incorrect or clunky. To be clear, there is no implementation in StreamIdProvider so I don't think this concept applies.

dynamic_cast is one option that is designed for safe downcasting.

Yes, this is an option but requires that we make HttpStreamIdProviderImpl public, it's currently private and IMO doesn't need to be made public.

i want to be a bit cautious here as it touches Envoy core code/interface

Yes, this is a fair point. Is there someone else you would recommend to give their opinion? I can add them as a reviewer here too.

[tyxia]

I think StreamIdProvider does have implementation : The code here has function declaration and function definition/implementation ---- {return absl::nullopt;}

Basically ,getCodecStreamId is only applicable to HttpStreamIdProviderImpl, but it is now polluted to StreamIdProvider, StreamIdProviderImpl class . What does those extra functions/interface bring, especially when this can be avoided them cleanly.

When people read the code, they will ask the same question as Ali asked at the beginning. "why is this implementation returning nullopt? where would this be invoked from? " IMHO, A clean interface design probably should be self-explanatory rather than raising this question.

[reubent-goog]

To preface this, we're splitting hairs over a fairly trivial thing. In the end we just need to decide which nit is more important: StreamInfo's API or adding a function to StreamIdProvider that's not used by one of the subclasses.

What does those extra functions/interface bring, especially when we can avoid them cleanly

"Cleanly" is a subjective idea. To me, it's less clean from an API perspective to add a new getter/setter field for stream id in StreamInfo when it already has [get|set]StreamIdProvider, to you it seems like it's less clean from a code cleanliness perspective to add a function to an interface which is only relevant for one of the two subclasses. I don't really know how we normally decide these matters.

[tyxia]

I am not just from code cleanliness perspective, i am more from API perspective.

Here is stream ID from codec which is not same as StreamIdProvider that was designed for providing string view for logging and tracing and integer view for in modulo, etc. calculations. I am fine with extending it but needing nullopt implementation makes me wondering if it is still good to be extended. I also feel "why is this implementation returning nullopt? where would this be invoked from? " is a very legit question to API design.

But again I don't have very strong opinion if you both feel current interface is good.

[ravenblackx]

Ping @abeyad (for after vacation)

[tyxia]

Thanks for working on this!

Mostly LGTM module some comments.

[tyxia]

Given that getCodecStreamId is only implemented and used in HttpStreamIdProviderImpl, can this function change to be a local member function of HttpStreamIdProviderImpl class?

[abeyad]

/wait

[abeyad]

Thanks Reuben! You'll need an Envoy maintainer to review, Tianyu is a maintainer so once he approves, it can be merged.