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

Introduce withParentOnly for @Observed #3843

Closed
wants to merge 1 commit into from

Conversation

quaff
Copy link
Contributor

@quaff quaff commented May 17, 2023

Take tracing for example, we may no want to create root span by @observed, they should be created if a root http span present.

@@ -52,4 +53,9 @@
*/
String[] lowCardinalityKeyValues() default {};

/**
* @return {@code true} if {@link Observation} should not be started without parent.
Copy link
Contributor

Choose a reason for hiding this comment

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

18% of developers fix this issue

MissingSummary: A summary fragment is required; consider using the value of the @return block as a summary fragment instead.


Suggested change
* @return {@code true} if {@link Observation} should not be started without parent.
*Returns {@code true} if {@link Observation} should not be started without parent.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

@sonatype-lift
Copy link
Contributor

sonatype-lift bot commented May 17, 2023

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.1

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/micrometer-metrics/micrometer/3843.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/micrometer-metrics/micrometer/3843.diff | git apply

Once you're satisfied, commit and push your changes in your project.

Footnotes

  1. You can preview the patch by opening the patch URL in the browser.

@marcingrzejszczak
Copy link
Contributor

Can you please elaborate on this? What problem are you trying to solve? What's currently not working?

@jonatan-ivanov jonatan-ivanov added the waiting for feedback We need additional information before we can continue label May 17, 2023
@quaff
Copy link
Contributor Author

quaff commented May 18, 2023

Can you please elaborate on this? What problem are you trying to solve? What's currently not working?

I don't want span created by @observed as root of trace, It must be child of other span.
For example, it shouldn't be started if called by @Scheduled trigger, but I want it be included in a complete http trace.

Take tracing for example, we may no want to create root span by @observed, they should be created if a root http span present.
@jonatan-ivanov
Copy link
Member

jonatan-ivanov commented May 18, 2023

I'm not sure I clearly get what you are trying to do so I'm going to make assumptions. Based on this:

For example, it shouldn't be started if called by @scheduled trigger, but I want it be included in a complete http trace.

I think you have a method that you annotate with @Observed and that method is called from different places, e.g.: from @Scheduled and from a @Controller. So what you need is basically a conditional @Observed.

I think there are two options to do this:

  1. Create an ObservationPredicate that checks if there is in a context of http and ignores it if it isn't. Here's an example, it's a little bit different but it can detect if your observation is in the context of http.
  2. Do not put @Observed on your @Scheduled method and use Observation.createNotStarted(...).observe(methodReference); in your @Controller instead.

I think this issue is related: #3678 and I would rather solve this for any Observation than only for @Observed.
What do you think?

@quaff
Copy link
Contributor Author

quaff commented May 19, 2023

I think you have a method that you annotate with @Observed and that method is called from different places, e.g.: from @Scheduled and from a @Controller. So what you need is basically a conditional @Observed.

You are right.

I think there are two options to do this:

  1. Create an ObservationPredicate that checks if there is in a context of http and ignores it if it isn't. Here's an example, it's a little bit different but it can detect if your observation is in the context of http.
  2. Do not put @Observed on your @Scheduled method and use Observation.createNotStarted(...).observe(methodReference); in your @Controller instead.

I vote for option 1, option 2 is viable but invasive.

I think compare to this PR there is a little drawback of ObservationPredicate, objects are created and then dropped, it will increase overhead, hope it's insignificant.

@jonatan-ivanov
Copy link
Member

I think compare to this PR there is a little drawback of ObservationPredicate, objects are created and then dropped, it will increase overhead, hope it's insignificant.

You should have only one ObservationPredicate instance that does this so this means one extra object in your whole application lifecycle. Also, using an ObservationPredicate should be more lightweight than this PR since in this PR a new Observation.Context and a new Observation will be created for every call (and dropped). With a predicate only the Observation.Context will be created (and dropped) there will not be a new Observation instance (we cannot spare creating the context since that's what your decision is based of).

@quaff
Copy link
Contributor Author

quaff commented May 31, 2023

Please close this if #3867 get merged, then we could use ObservationPredicate like this:

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

@marcingrzejszczak
Copy link
Contributor

Closing since we did this #3867

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for feedback We need additional information before we can continue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants