Skip to content

Conversation

j-denner
Copy link
Contributor

@j-denner j-denner commented Jul 9, 2024

AddHttpHeaderToLogContextFilter: Filter immediately for getField presence

  • Previously the present of a field name was checked after fetching the header value and then dropped.
    This can be done once and during initialization.

…ence

* Previously the present of a field name was checked after fetchin the header value and then dropped.
  This can be done once and during initialization.
Copy link
Contributor

@KarstenSchnitter KarstenSchnitter left a comment

Choose a reason for hiding this comment

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

Thanks for providing this change. The code has probably little affect. The point of an HttpHeader is to provide a mapping of a header to a field, when isPropagated is set to true. The null check was put in place to avoid NPEs.

I am wondering, whether the filter should issue an INFO level log, when it encounters a "propagated" header without a field. I would see this as a violation of the HttpHeader contract.

@j-denner
Copy link
Contributor Author

j-denner commented Jul 9, 2024

I phrase it the other way round. There is the invariant propagated=true => field != null that is currently not enforced in any location. Such a check could be added, but then it should be introduce in all places that accept HttpHeader as a configuration. Since HttpHeader is an interface consumers may have derived classes that do no fulfill this contract.

The code has probably little affect.

I think no. When field is null, the retrieved header value is ignored. So the early filtering reduces work - like for fields.

@j-denner
Copy link
Contributor Author

j-denner commented Jul 9, 2024

I don't known how many breaking changes you 'dare' with the next major release. The HttpHeader interface could be changed to a sealed one, that allows the current Enum and a new HttpHeaderRecord record class. The latter can enforce the contract and consumers can still create own Headers. Just some thoughts.

@KarstenSchnitter
Copy link
Contributor

For the next major release, I am thinking of moving the HttpHeader interface from the core module to the servlet module. This would already be a breaking change. Implementing your suggestion would be an additional and possible option.

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