You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/11/30 16:06:21 UTC
[GitHub] [skywalking-java] marcingrzejszczak opened a new pull request, #401: Adds Micrometer Observation instrumentation
marcingrzejszczak opened a new pull request, #401:
URL: https://github.com/apache/skywalking-java/pull/401
<!--
β οΈ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist π ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== π Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist π ==== -->
### Add an agent plugin to support <framework name>
- [X] Add a test case for the new plugin, refer to [the doc](https://github.com/apache/skywalking-java/blob/main/docs/en/setup/service-agent/java-agent/Plugin-test.md)
- [ ] Add a component id in [the component-libraries.yml](https://github.com/apache/skywalking/blob/master/oap-server/server-starter/src/main/resources/component-libraries.yml)
- [ ] Add a logo in [the UI repo](https://github.com/apache/skywalking-rocketbot-ui/tree/master/src/views/components/topology/assets)
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist π ====
### Improve the performance of <class or module or ...>
- [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking-java/blob/main/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
- [ ] The benchmark result.
```text
<Paste the benchmark results here>
```
- [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
==== π Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist π ==== -->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist π ====
### <Feature description>
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
==== π Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist π ==== -->
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking-java/blob/main/CHANGES.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] yswdqz commented on pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#issuecomment-1333106840
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#discussion_r1037040040
##########
test/plugin/scenarios/resttemplate-6.x-scenario/config/expectedData.yaml:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+segmentItems:
+- serviceName: resttemplate-6.x-scenario
+ segmentSize: ge 2
+ segments:
+ - segmentId: not null
+ spans:
+ - operationName: http head /resttemplate/case/healthcheck
+ parentSpanId: -1
+ spanId: 0
+ spanLayer: Http
+ startTime: nq 0
+ endTime: nq 0
+ componentId: nq 0
+ isError: false
+ spanType: Entry
+ peer: '/resttemplate-6.x-scenario/resttemplate/case/healthcheck'
+ tags:
+ - {key: http.url, value: '/resttemplate-6.x-scenario/resttemplate/case/healthcheck'}
+ - {key: method, value: HEAD}
+ - {key: status, value: '200'}
+ skipAnalysis: 'false'
+ - segmentId: not null
+ spans:
+ - operationName: http get /resttemplate/syncback
+ parentSpanId: -1
+ spanId: 0
+ spanLayer: Http
+ startTime: nq 0
+ endTime: nq 0
+ componentId: nq 0
+ isError: false
+ spanType: Entry
+ peer: '/resttemplate-6.x-scenario/resttemplate/syncback'
+ tags:
+ - {key: http.url, value: '/resttemplate-6.x-scenario/resttemplate/syncback'}
+ - {key: method, value: GET}
+ - {key: status, value: '200'}
+ skipAnalysis: 'false'
+ - segmentId: not null
+ spans:
+ - operationName: http get
Review Comment:
I think this span should be in the segment with another entry span(`/resttemplate/case/resttemplate`)?
There should be 2 segments with 3 spans to be verified
Segment <1>, entry span for `entryService`(in the configuration.yml`, and exit span for client-side calling
Segment <2>, the `/syncback` with a ref pointing to the segment<1>'s client-side calling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] marcingrzejszczak commented on pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
marcingrzejszczak commented on PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#issuecomment-1333566934
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#discussion_r1037037695
##########
test/plugin/scenarios/resttemplate-6.x-scenario/config/expectedData.yaml:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+segmentItems:
+- serviceName: resttemplate-6.x-scenario
+ segmentSize: ge 2
+ segments:
+ - segmentId: not null
+ spans:
+ - operationName: http head /resttemplate/case/healthcheck
+ parentSpanId: -1
+ spanId: 0
+ spanLayer: Http
+ startTime: nq 0
+ endTime: nq 0
+ componentId: nq 0
+ isError: false
+ spanType: Entry
+ peer: '/resttemplate-6.x-scenario/resttemplate/case/healthcheck'
+ tags:
+ - {key: http.url, value: '/resttemplate-6.x-scenario/resttemplate/case/healthcheck'}
+ - {key: method, value: HEAD}
+ - {key: status, value: '200'}
+ skipAnalysis: 'false'
+ - segmentId: not null
+ spans:
+ - operationName: http get /resttemplate/syncback
Review Comment:
We should check the ref in this span.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] marcingrzejszczak commented on a diff in pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
marcingrzejszczak commented on code in PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#discussion_r1039031375
##########
apm-application-toolkit/apm-toolkit-micrometer-registry/src/main/java/org/apache/skywalking/apm/toolkit/micrometer/observation/SkywalkingContextSnapshotThreadLocalAccessor.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.toolkit.micrometer.observation;
+
+import io.micrometer.context.ThreadLocalAccessor;
+
+/**
+ * A {@link ThreadLocalAccessor} to put and restore current {@code ContextSnapshot} from APM agent.
+ */
+public class SkywalkingContextSnapshotThreadLocalAccessor implements ThreadLocalAccessor<Object> {
Review Comment:
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#issuecomment-1333563762
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] mrproliu commented on a diff in pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
mrproliu commented on code in PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#discussion_r1038921845
##########
apm-application-toolkit/apm-toolkit-micrometer-registry/src/main/java/org/apache/skywalking/apm/toolkit/micrometer/observation/SkywalkingContextSnapshotThreadLocalAccessor.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.toolkit.micrometer.observation;
+
+import io.micrometer.context.ThreadLocalAccessor;
+
+/**
+ * A {@link ThreadLocalAccessor} to put and restore current {@code ContextSnapshot} from APM agent.
+ */
+public class SkywalkingContextSnapshotThreadLocalAccessor implements ThreadLocalAccessor<Object> {
Review Comment:
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.
##########
apm-sniffer/apm-toolkit-activation/apm-toolkit-micrometer-activation/src/main/java/org/apache/skywalking/apm/toolkit/activation/micrometer/MicrometerDefaultTracingHandlerInstrumentation.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.toolkit.activation.micrometer;
+
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.ConstructorInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.InstanceMethodsInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassInstanceMethodsEnhancePluginDefine;
+import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch;
+import org.apache.skywalking.apm.agent.core.plugin.match.NameMatch;
+
+import static net.bytebuddy.matcher.ElementMatchers.named;
+
+public class MicrometerDefaultTracingHandlerInstrumentation extends ClassInstanceMethodsEnhancePluginDefine {
+
+ public static final String ENHANCE_CLASS = "org.apache.skywalking.apm.toolkit.micrometer.observation.SkywalkingDefaultTracingHandler";
+
+ public static final String INTERCEPT_START_POINT_METHOD = "onStart";
+
+ public static final String INTERCEPT_STOP_POINT_METHOD = "onStop";
+
+ public static final String INTERCEPT_ERROR_POINT_METHOD = "onError";
+
+ public static final String INTERCEPT_EVENT_POINT_METHOD = "onEvent";
+
+ public static final String INTERCEPT_CLASS = "org.apache.skywalking.apm.plugin.micrometer.MicrometerDefaultTracingHandlerInterceptor";
Review Comment:
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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] mrproliu commented on a diff in pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
mrproliu commented on code in PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#discussion_r1039066157
##########
apm-application-toolkit/apm-toolkit-micrometer-registry/src/main/java/org/apache/skywalking/apm/toolkit/micrometer/observation/SkywalkingContextSnapshotThreadLocalAccessor.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.toolkit.micrometer.observation;
+
+import io.micrometer.context.ThreadLocalAccessor;
+
+/**
+ * A {@link ThreadLocalAccessor} to put and restore current {@code ContextSnapshot} from APM agent.
+ */
+public class SkywalkingContextSnapshotThreadLocalAccessor implements ThreadLocalAccessor<Object> {
Review Comment:
> 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](https://github.com/apache/skywalking-java/blob/main/apm-sniffer/apm-toolkit-activation/apm-toolkit-trace-activation/src/main/java/org/apache/skywalking/apm/toolkit/activation/trace/CallableOrRunnableActivation.java), you could check this out. In this demo, we capture a snapshot from the tracing context and continue the snapshot in other thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng merged pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #401:
URL: https://github.com/apache/skywalking-java/pull/401
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] marcingrzejszczak commented on pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
marcingrzejszczak commented on PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#issuecomment-1336154740
Skywalking has nothing to do with Sleuth. There is no Sleuth meter config, there's only micrometer meter config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] marcingrzejszczak commented on a diff in pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
marcingrzejszczak commented on code in PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#discussion_r1039031411
##########
apm-sniffer/apm-toolkit-activation/apm-toolkit-micrometer-activation/src/main/java/org/apache/skywalking/apm/toolkit/activation/micrometer/MicrometerDefaultTracingHandlerInstrumentation.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.toolkit.activation.micrometer;
+
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.ConstructorInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.InstanceMethodsInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassInstanceMethodsEnhancePluginDefine;
+import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch;
+import org.apache.skywalking.apm.agent.core.plugin.match.NameMatch;
+
+import static net.bytebuddy.matcher.ElementMatchers.named;
+
+public class MicrometerDefaultTracingHandlerInstrumentation extends ClassInstanceMethodsEnhancePluginDefine {
+
+ public static final String ENHANCE_CLASS = "org.apache.skywalking.apm.toolkit.micrometer.observation.SkywalkingDefaultTracingHandler";
+
+ public static final String INTERCEPT_START_POINT_METHOD = "onStart";
+
+ public static final String INTERCEPT_STOP_POINT_METHOD = "onStop";
+
+ public static final String INTERCEPT_ERROR_POINT_METHOD = "onError";
+
+ public static final String INTERCEPT_EVENT_POINT_METHOD = "onEvent";
+
+ public static final String INTERCEPT_CLASS = "org.apache.skywalking.apm.plugin.micrometer.MicrometerDefaultTracingHandlerInterceptor";
Review Comment:
I guess I'll need to move the classes around again after talking to @wu-sheng but thanks for pointing this out
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#discussion_r1037035658
##########
test/plugin/scenarios/resttemplate-6.x-scenario/config/expectedData.yaml:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+segmentItems:
+- serviceName: resttemplate-6.x-scenario
+ segmentSize: ge 2
+ segments:
+ - segmentId: not null
+ spans:
+ - operationName: http head /resttemplate/case/healthcheck
+ parentSpanId: -1
+ spanId: 0
+ spanLayer: Http
+ startTime: nq 0
+ endTime: nq 0
+ componentId: nq 0
+ isError: false
+ spanType: Entry
+ peer: '/resttemplate-6.x-scenario/resttemplate/case/healthcheck'
+ tags:
+ - {key: http.url, value: '/resttemplate-6.x-scenario/resttemplate/case/healthcheck'}
+ - {key: method, value: HEAD}
+ - {key: status, value: '200'}
+ skipAnalysis: 'false'
+ - segmentId: not null
+ spans:
+ - operationName: http get /resttemplate/syncback
+ parentSpanId: -1
+ spanId: 0
+ spanLayer: Http
+ startTime: nq 0
+ endTime: nq 0
+ componentId: nq 0
+ isError: false
+ spanType: Entry
+ peer: '/resttemplate-6.x-scenario/resttemplate/syncback'
+ tags:
+ - {key: http.url, value: '/resttemplate-6.x-scenario/resttemplate/syncback'}
+ - {key: method, value: GET}
+ - {key: status, value: '200'}
+ skipAnalysis: 'false'
+ - segmentId: not null
+ spans:
+ - operationName: http get
+ parentSpanId: 0
+ spanId: 1
+ spanLayer: Http
+ startTime: nq 0
+ endTime: nq 0
+ componentId:
+ isError: false
+ spanType: Exit
+ peer: http://localhost:8080/resttemplate-6.x-scenario/resttemplate/syncback
Review Comment:
Peer should be remote addr only. Using `http.url` is working, but it increases the backend analysis payload a lot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] marcingrzejszczak commented on pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
marcingrzejszczak commented on PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#issuecomment-1333561279
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#discussion_r1037038196
##########
test/plugin/scenarios/resttemplate-6.x-scenario/config/expectedData.yaml:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+segmentItems:
+- serviceName: resttemplate-6.x-scenario
+ segmentSize: ge 2
+ segments:
+ - segmentId: not null
+ spans:
+ - operationName: http head /resttemplate/case/healthcheck
Review Comment:
`/healthcheck` is not required to verify.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#discussion_r1037040040
##########
test/plugin/scenarios/resttemplate-6.x-scenario/config/expectedData.yaml:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+segmentItems:
+- serviceName: resttemplate-6.x-scenario
+ segmentSize: ge 2
+ segments:
+ - segmentId: not null
+ spans:
+ - operationName: http head /resttemplate/case/healthcheck
+ parentSpanId: -1
+ spanId: 0
+ spanLayer: Http
+ startTime: nq 0
+ endTime: nq 0
+ componentId: nq 0
+ isError: false
+ spanType: Entry
+ peer: '/resttemplate-6.x-scenario/resttemplate/case/healthcheck'
+ tags:
+ - {key: http.url, value: '/resttemplate-6.x-scenario/resttemplate/case/healthcheck'}
+ - {key: method, value: HEAD}
+ - {key: status, value: '200'}
+ skipAnalysis: 'false'
+ - segmentId: not null
+ spans:
+ - operationName: http get /resttemplate/syncback
+ parentSpanId: -1
+ spanId: 0
+ spanLayer: Http
+ startTime: nq 0
+ endTime: nq 0
+ componentId: nq 0
+ isError: false
+ spanType: Entry
+ peer: '/resttemplate-6.x-scenario/resttemplate/syncback'
+ tags:
+ - {key: http.url, value: '/resttemplate-6.x-scenario/resttemplate/syncback'}
+ - {key: method, value: GET}
+ - {key: status, value: '200'}
+ skipAnalysis: 'false'
+ - segmentId: not null
+ spans:
+ - operationName: http get
Review Comment:
I think this span should be in the segment with another entry span(`/resttemplate/case/resttemplate`)?
There should be 2 segments with 3 spans to be verified
Segment <1>, entry span for `entryService`(in the Β·configuration.yml`, and exit span for client-side calling
Segment <2>, the `/syncback` with a ref pointing to the segment<1>'s client-side calling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] marcingrzejszczak commented on pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
marcingrzejszczak commented on PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#issuecomment-1335400346
I've managed to finally set up the e2e scenario with Spring Framework 6, jdk 17.
In Skywalking got the following graph:
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}]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#issuecomment-1336153026
@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](https://github.com/apache/skywalking/tree/18008aa6b24ada6602d9c73e1a9d19b6105d8b59/oap-server/server-starter/src/main/resources/meter-analyzer-config/spring-sleuth.yaml). If you have a customized meter at the agent side, please configure the meter using the steps set out in the [meter document](https://skywalking.apache.org/docs/main/next/en/setup/backend/backend-meter#meters-configure).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] marcingrzejszczak commented on pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
marcingrzejszczak commented on PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#issuecomment-1385039295
I will do it asap, thanks :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#issuecomment-1384881186
@marcingrzejszczak We have 8.14.0 released now. Could you try to make your plugin test back to the CI pipeline?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#discussion_r1039056176
##########
apm-application-toolkit/apm-toolkit-micrometer-registry/src/main/java/org/apache/skywalking/apm/toolkit/micrometer/observation/SkywalkingContextSnapshotThreadLocalAccessor.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.toolkit.micrometer.observation;
+
+import io.micrometer.context.ThreadLocalAccessor;
+
+/**
+ * A {@link ThreadLocalAccessor} to put and restore current {@code ContextSnapshot} from APM agent.
+ */
+public class SkywalkingContextSnapshotThreadLocalAccessor implements ThreadLocalAccessor<Object> {
Review Comment:
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #401: Adds Micrometer Observation instrumentation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #401:
URL: https://github.com/apache/skywalking-java/pull/401#issuecomment-1332969501
@pg-yang @yswdqz Does any of you have time to continue on this one? Marcin finished most parts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org