Make Context.getParentObservation() available for ObservationPredicate

[quaff]

After this commit, Context.setParentObservation() is called before matching predicate.
now we can filter out parentless observations like this:

	@Bean
	ObservationPredicate noParentlessDatabaseObservations() {
		return (name, context) -> {
			if (context instanceof LettuceObservationContext || context instanceof DataSourceBaseContext) {
				return context.getParentObservation() != null;
			}
			return true;
		};
	}

Resolves #3678

[jonatan-ivanov]

Thank you for the PR! Let us discuss this internally, I think this could resolve #3678 also we might want to add something like a getRootObservation method to get the first Observation in the chain conveniently.

[quaff]

Thank you for the PR! Let us discuss this internally, I think this could resolve #3678 also we might want to add something like a getRootObservation method to get the first Observation in the chain conveniently.

Should I add getRootObservation in this PR?

[quaff]

Would it be merged in next release?

[quaff]

I think it like a bug more than a enhancement, please fix it ASAP.

[shakuzen]

The changes seem reasonable to me. One minor concern is that moving this out of the constructor means that we need to always make sure to set the parent wherever we might create an Observation. Even if this pull request covers all the places now, it's something that could be missed in future changes. I'm not sure we can do anything about that and allow the behavior we're seeking to change.
I'm asking others on the team to review so we can get feedback on this change in snapshots and a future milestone if we are going to merge this.

[quaff]

The changes seem reasonable to me. One minor concern is that moving this out of the constructor means that we need to always make sure to set the parent wherever we might create an Observation. Even if this pull request covers all the places now, it's something that could be missed in future changes. I'm not sure we can do anything about that and allow the behavior we're seeking to change. I'm asking others on the team to review so we can get feedback on this change in snapshots and a future milestone if we are going to merge this.

We could revert SimpleObservation.java to make sure that, but it's safe to remove the redundant code since SimpleObservation is not public. @shakuzen

[shakuzen]

The team discussed and we're okay to merge now and get some feedback and testing from users of this code. There are several consumers of this code that we'll want to get feedback on post-merge. I've built this locally and ran the tracing build against it - the build passed. Some instrumentation may be able to get rid of some code thanks to this change if they are manually setting a parent on the context before creating an Observation.

Thank you @quaff for the pull request.

We could revert SimpleObservation.java to make sure that, but it's safe to remove the redundant code since SimpleObservation is not public.

Thanks for thinking about this. That idea did occur to me, but I think if we're hitting that code path, we're going to have a difference in behavior between that path and other paths that we probably don't want.

[quaff]

@shakuzen Thanks for your quick response, I've updated the commit to eliminate unnecessary null check context == null ? new Context() : context, you can polish it in another commit.

[shakuzen]

I've updated the commit to eliminate unnecessary null check context == null ? new Context() : context, you can polish it in another commit.

I didn't notice that. We should not have changed that in this pull request. It's not good that we didn't have tests that failed because of the change. The context supplier comes from users, so we can't know that it doesn't return null, and we have no way of marking nullability of what the supplier returns. While isObservationEnabled takes a nullable context, the SimpleObservation constructor does not. But since we're using the context before either of those now, we need to figure out what to do.

[Nirshal]

Any estimate on which release this will be included in?

[jonatan-ivanov]

@Nirshal If you look at the right side of the UI in the Development section, an issue is linked to this PR. That issue has a milestone: 12.0.0-M3. The milestone should have a due date (it did not have it, I just set it): 2023-09-11.
It's not guaranteed that we will release it on that day but that's the date we are aiming for.

If you are asking about the GA release date that should be in November (I guess 13th).

You can also check: https://calendar.spring.io/

[Nirshal]

@jonatan-ivanov thanks a lot for the explanation!