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

Make observation return its context and immutable access to parent #3423

Merged
merged 2 commits into from Sep 20, 2022

Conversation

ttddyy
Copy link
Contributor

@ttddyy ttddyy commented Sep 19, 2022

This PR changes Observation#getContext() to return Context. Then, callers can obtain the actual Context object from observation and perform any update on it.

To prevent users modify the parent observation context, this PR introduces ObservationView interface. The interface provides a method that returns ContextView for the observation.
Then, the Observation.Context#getParentObservation() is updated to return ObservationView instead of the parent Observation.
By the API contract, users cannot modify the parent observation context.

The getParentObservation() return type change is a small breaking change on API.
However, modifying the parent observation is a bad practice. Nobody should be doing it and the impact should be minimal.

@ttddyy ttddyy changed the title Make Observation#getContext() return Context Make observation return its context and immutable access to parent Sep 19, 2022
To prevent modifying the parent observation, introduces the
`ObservationView`, a read only view of the observation.  Then, makes
`Observation.Context#getParentObservation` to return the
`ObservationView` which gives immutable way of referencing the context.
@ttddyy ttddyy marked this pull request as ready for review September 19, 2022 22:03
@hadjiski
Copy link

@ttddyy Hi, I came across your PR, which hinders me to accomplish something I had in mind, which I read out from your statement now is a bad practice. Pls tell me, how to achieve it then otherwise. Here is the use case:
Normal Spring boot 3 application with a rest controller, business layer and a spring data repository involving mongodb driver.
This ends up having the following observations auto-configured (among others):
http.server.requests
http.client.requests
spring.data.repository.invocations
mongodb.driver.commands

For all besides the "http.server.requests" the current observation is the "spring.security.http.secured.requests" with parent "spring.security.filterchains" with root parent "http.server.requests"

The "http.server.requests" has no parent.

Now when I customize tags for these metrics, typically some beans are overridden or interfaces implemented and inside we either have access to the observation context (e.g. for the http.server/client.requests) or not (e.g. for the spring data observations), so one can not write nicely a reusable code how to propagate business data from the business layer into the context and read it out inside the customizers to decide on the custom tags.
So my idea was to consistently put my business data into the ObservationRegistry.getCurrentObservation().getContext().put(...) and then inside the customizer to read it out, but here is the problem:
Inside the customizer for http.server.requests the current observation context is a parent of the one, which was available at the time the business data was propagated into the context, so I was thinking, why not propagating the business data always to the root parent, so that no matter which context is available in the customizers, I just need to lookup in their root parents and will find it.
Though you consider this as bad practice, so how to solve it else then?

Btw. here is concrete example:
In the rest controller I am putting business data into the current context "spring.security.http.secured.requests", then later in the customizer for http.server.requests I am only having access to the http.server.requests context, which is a parent, so no way to access the previously added business data

Thanks

@ttddyy ttddyy deleted the return-context branch March 31, 2024 17:16
@ttddyy
Copy link
Contributor Author

ttddyy commented Mar 31, 2024

@hadjiski
Since this issue is old and already closed/merged, please create a new one about the parent context immutability, linking to this issue.
I'll put my comment there.
Thanks,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants