Skip to content

Adds Micrometer Observation instrumentation#401

Merged
wu-sheng merged 22 commits intoapache:mainfrom
marcingrzejszczak:micrometer
Dec 6, 2022
Merged

Adds Micrometer Observation instrumentation#401
wu-sheng merged 22 commits intoapache:mainfrom
marcingrzejszczak:micrometer

Conversation

@marcingrzejszczak
Copy link
Copy Markdown
Contributor

@marcingrzejszczak marcingrzejszczak commented Nov 30, 2022

Add an agent plugin to support

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
  • Update the CHANGES log.

@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Dec 1, 2022

@pg-yang @yswdqz Does any of you have time to continue on this one? Marcin finished most parts.

@yswdqz
Copy link
Copy Markdown
Member

yswdqz commented Dec 1, 2022

@pg-yang @yswdqz Does any of you have time to continue on this one? Marcin finished most parts.

Sorry, I don't have time recently.

@marcingrzejszczak
Copy link
Copy Markdown
Contributor Author

The only thing to look at is the e2e plugin tests and modify the expectedData.yml for the tests to pass.

Also I see that we're currently in Spring naming spans in a different way than skywalking does. Should we leave it like this or rename the spans in the way that Skywalking does it?

@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Dec 1, 2022

Also I see that we're currently in Spring naming spans in a different way than skywalking does. Should we leave it like this or rename the spans in the way that Skywalking does it?

It is your call. URI style or ./spacing are helpful for endpoint searching. The endpoint is from span name of entry span.

@marcingrzejszczak
Copy link
Copy Markdown
Contributor Author

Ok, the user can rename the observations at runtime by providing their own ObservationFilter to the registry, so that is not a problem.

So the only thing that remains is to make the e2e pass and we're done.

Comment thread test/plugin/scenarios/resttemplate-6.x-scenario/config/expectedData.yaml Outdated
Comment thread test/plugin/scenarios/resttemplate-6.x-scenario/config/expectedData.yaml Outdated
Comment thread test/plugin/scenarios/resttemplate-6.x-scenario/config/expectedData.yaml Outdated
Comment thread test/plugin/scenarios/resttemplate-6.x-scenario/config/expectedData.yaml Outdated
@marcingrzejszczak
Copy link
Copy Markdown
Contributor Author

I've managed to finally set up the e2e scenario with Spring Framework 6, jdk 17.

In Skywalking got the following graph:

image

The headers get propagated

Got following headers [sw8:"1-ZDA3YTFjNzc4YWEzNDc3NWFmY2Q2MjQ3ZWY0N2ZiMzIuNTYuMTY2OTk5MzEzNzk1NjAwMDE=-ZDA3YTFjNzc4YWEzNDc3NWFmY2Q2MjQ3ZWY0N2ZiMzIuNTYuMTY2OTk5MzEzNzk1NjAwMDA=-1-WW91cl9BcHBsaWNhdGlvbk5hbWU=-MmFiMWNkNWRmZTJmNDJhNDkzNTA3ZjFkNDhiZTJmMTFAMTAuNy4xOC4xOTE=-aHR0cC5zZXJ2ZXIucmVxdWVzdHM=-bG9jYWxob3N0OjIzNDU=", sw8-correlation:"", sw8-x:"0-", accept:"text/plain, application/json, application/*+json, */*", user-agent:"Java/17.0.1", host:"localhost:2345", connection:"keep-alive"]

And the metrics are collected

 - Metric type  [TIMER],        name [http.client.requests],    tags [tag(error=none), tag(exception=none), tag(method=GET), tag(outcome=SUCCESS), tag(status=200), tag(uri=http://localhost:2345/resttemplate-6.x-scenario/resttemplate/syncback)],      measurements [Measurement{statistic='COUNT', value=1.0}, Measurement{statistic='TOTAL_TIME', value=123511.0}, Measurement{statistic='MAX', value=123511.0}]
 - Metric type  [LONG_TASK_TIMER],      name [http.server.requests.active],     tags [tag(exception=none), tag(method=GET), tag(outcome=SUCCESS), tag(status=200), tag(uri=UNKNOWN)],     measurements [Measurement{statistic='ACTIVE_TASKS', value=0.0}, Measurement{statistic='DURATION', value=0.0}]
 - Metric type  [TIMER],        name [http.server.requests],    tags [tag(error=none), tag(exception=none), tag(method=GET), tag(outcome=SUCCESS), tag(status=200), tag(uri=/resttemplate/syncback)],     measurements [Measurement{statistic='COUNT', value=1.0}, Measurement{statistic='TOTAL_TIME', value=68701.0}, Measurement{statistic='MAX', value=68701.0}]
 - Metric type  [LONG_TASK_TIMER],      name [http.client.requests.active],     tags [tag(exception=none), tag(method=GET), tag(outcome=UNKNOWN), tag(status=CLIENT_ERROR), tag(uri=http://localhost:2345/resttemplate-6.x-scenario/resttemplate/syncback)],      measurements [Measurement{statistic='ACTIVE_TASKS', value=0.0}, Measurement{statistic='DURATION', value=0.0}]
 - Metric type  [TIMER],        name [http.server.requests],    tags [tag(error=none), tag(exception=none), tag(method=GET), tag(outcome=SUCCESS), tag(status=200), tag(uri=/resttemplate/case/resttemplate)],    measurements [Measurement{statistic='COUNT', value=1.0}, Measurement{statistic='TOTAL_TIME', value=149568.0}, Measurement{statistic='MAX', value=149568.0}]

@wu-sheng wu-sheng added this to the 8.14.0 milestone Dec 2, 2022
@wu-sheng wu-sheng removed the TBD label Dec 2, 2022
marcingrzejszczak added a commit to marcingrzejszczak/skywalking that referenced this pull request Dec 2, 2022
@wu-sheng wu-sheng requested a review from mrproliu December 3, 2022 12:39
@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Dec 3, 2022

@mrproliu Could you recheck this as an enhancement of the Micrometer?

BTW, from @marcingrzejszczak , the sleuth is not there, but micrometer still is. https://skywalking.apache.org/docs/main/next/en/setup/backend/spring-sleuth-setup/ the doc should be updated.

@marcingrzejszczak What is your suggestion for this part of the doc? Are these metrics still valid?

Configure the meter config file. It already has the spring sleuth meter config. If you have a customized meter at the agent side, please configure the meter using the steps set out in the meter document.

@marcingrzejszczak
Copy link
Copy Markdown
Contributor Author

Skywalking has nothing to do with Sleuth. There is no Sleuth meter config, there's only micrometer meter config.

/**
* A {@link ThreadLocalAccessor} to put and restore current {@code ContextSnapshot} from APM agent.
*/
public class SkywalkingContextSnapshotThreadLocalAccessor implements ThreadLocalAccessor<Object> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When do we need this? I see the agent code, it used to continue the context when cross process? But there also have some TODO not finished.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually I would need some feedback from you. When we continue a span between threads, what should be the name of that continued span? Also how can you reset the thread local with a span (how can I remove the active span from thread local)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Usually, we don't continue a span in a new thread. We only support finishing a span in a new thread, or continuous tracing context(capture and continuous snapshot from ContextManager)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When we continue a span between threads, what should be the name of that continued span?

It's usually decided by when you need to cross-thread. Such as when we submit a new thread, we called Thread/{thread_name}/{running_function}.

Also how can you reset the thread local with a span (how can I remove the active span from thread local)

Here has a good demo, you could check this out. In this demo, we capture a snapshot from the tracing context and continue the snapshot in other thread.


public static final String INTERCEPT_EVENT_POINT_METHOD = "onEvent";

public static final String INTERCEPT_CLASS = "org.apache.skywalking.apm.plugin.micrometer.MicrometerDefaultTracingHandlerInterceptor";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The interceptor classes of all instrumentation are not correct, please help to fix it.
In this case, it should be: org.apache.skywalking.apm.toolkit.activation.micrometer.MicrometerDefaultTracingHandlerInterceptor

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess I'll need to move the classes around again after talking to @wu-sheng but thanks for pointing this out

@wu-sheng
Copy link
Copy Markdown
Member

@marcingrzejszczak We have 8.14.0 released now. Could you try to make your plugin test back to the CI pipeline?

@marcingrzejszczak
Copy link
Copy Markdown
Contributor Author

I will do it asap, thanks :)

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants