wu-sheng commented on code in PR #808: URL: https://github.com/apache/skywalking-java/pull/808#discussion_r3341381841
########## apm-sniffer/apm-sdk-plugin/spring-plugins/spring-ai-1.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/spring/ai/v1/AbstractObservationVectorStoreInterceptor.java: ########## @@ -0,0 +1,162 @@ +/* + * 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.plugin.spring.ai.v1; + +import org.apache.skywalking.apm.agent.core.context.ContextManager; +import org.apache.skywalking.apm.agent.core.context.tag.Tags; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; +import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult; +import org.apache.skywalking.apm.agent.core.util.GsonUtil; +import org.apache.skywalking.apm.network.trace.component.ComponentsDefine; +import org.apache.skywalking.apm.plugin.spring.ai.v1.contant.Constants; +import org.springframework.ai.document.Document; +import org.springframework.ai.vectorstore.SearchRequest; +import org.springframework.ai.vectorstore.observation.AbstractObservationVectorStore; +import org.springframework.ai.vectorstore.observation.VectorStoreObservationContext; +import org.springframework.util.StringUtils; + +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +public class AbstractObservationVectorStoreInterceptor implements InstanceMethodsAroundInterceptor { + + @Override + public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, + MethodInterceptResult result) throws Throwable { + SearchRequest request = (SearchRequest) allArguments[0]; + String dataSourceId = objInst.getClass().getSimpleName(); + + try { + VectorStoreObservationContext context = + createObservationContext(objInst, request); + + String resolved = + resolveDataSourceId(context, objInst); + + if (StringUtils.hasText(resolved)) { + dataSourceId = resolved; + } + } catch (Throwable ignored) { + + } + + AbstractSpan span = ContextManager.createExitSpan(Constants.RETRIEVAL + "/" + dataSourceId, dataSourceId); + + SpanLayer.asGenAI(span); + span.setComponent(ComponentsDefine.SPRING_AI); + Tags.GEN_AI_OPERATION_NAME.set(span, Constants.RETRIEVAL); + Tags.GEN_AI_DATA_SOURCE_ID.set(span, dataSourceId); + String model = resolveEmbeddingModelName(objInst); + if (StringUtils.hasText(model)) { + Tags.GEN_AI_REQUEST_MODEL.set(span, model); + } + + if (request != null) { + Tags.GEN_AI_TOP_K.set(span, String.valueOf(request.getTopK())); + if (StringUtils.hasText(request.getQuery())) { + Tags.GEN_AI_RETRIEVAL_QUERY_TEXT.set(span, request.getQuery()); Review Comment: The raw user query is captured on every search with no gate. The plugin otherwise suppresses sensitive content by default — `gen_ai.input.messages` / `output.messages` / tool args are all behind `COLLECT_*` flags that default to `false`, with a 1024-char cap. The RAG query is the same class of free-text user input, so this bypasses that posture. Suggest gating it behind a flag consistent with the existing ones (e.g. a new `COLLECT_RETRIEVAL_QUERY = false`, or reuse `COLLECT_INPUT_MESSAGES`) and applying a length limit like the chat path does. ########## apm-sniffer/apm-sdk-plugin/spring-plugins/spring-ai-1.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/spring/ai/v1/EmbeddingModelInterceptor.java: ########## @@ -0,0 +1,65 @@ +/* + * 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.plugin.spring.ai.v1; + +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult; +import org.springframework.ai.embedding.EmbeddingResponse; +import org.springframework.ai.embedding.EmbeddingResponseMetadata; +import org.springframework.util.StringUtils; + +import java.lang.reflect.Method; + +public class EmbeddingModelInterceptor implements InstanceMethodsAroundInterceptor { + + @Override + public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) { + + } + + @Override + public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) { + EmbeddingResponseMetadata metadata = ((EmbeddingResponse) ret).getMetadata(); Review Comment: `((EmbeddingResponse) ret).getMetadata()` runs with no guard on `ret`. The agent calls `afterMethod` from a `finally`, so it also runs when `call(EmbeddingRequest)` threw — and then `ret` is `null`, NPE-ing on every embedding failure (caught + logged by the framework, but it spams ERROR logs). `ChatModelCallInterceptor.afterMethod` guards with `if (!(ret instanceof ChatResponse)) { return ret; }` — suggest the same pattern here. The `metadata == null` check below then becomes dead, since `getMetadata()` never returns null. ########## apm-sniffer/apm-sdk-plugin/spring-plugins/spring-ai-1.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/spring/ai/v1/AbstractObservationVectorStoreInterceptor.java: ########## @@ -0,0 +1,162 @@ +/* + * 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.plugin.spring.ai.v1; + +import org.apache.skywalking.apm.agent.core.context.ContextManager; +import org.apache.skywalking.apm.agent.core.context.tag.Tags; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; +import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult; +import org.apache.skywalking.apm.agent.core.util.GsonUtil; +import org.apache.skywalking.apm.network.trace.component.ComponentsDefine; +import org.apache.skywalking.apm.plugin.spring.ai.v1.contant.Constants; +import org.springframework.ai.document.Document; +import org.springframework.ai.vectorstore.SearchRequest; +import org.springframework.ai.vectorstore.observation.AbstractObservationVectorStore; +import org.springframework.ai.vectorstore.observation.VectorStoreObservationContext; +import org.springframework.util.StringUtils; + +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +public class AbstractObservationVectorStoreInterceptor implements InstanceMethodsAroundInterceptor { + + @Override + public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, + MethodInterceptResult result) throws Throwable { + SearchRequest request = (SearchRequest) allArguments[0]; + String dataSourceId = objInst.getClass().getSimpleName(); + + try { + VectorStoreObservationContext context = + createObservationContext(objInst, request); + + String resolved = + resolveDataSourceId(context, objInst); + + if (StringUtils.hasText(resolved)) { + dataSourceId = resolved; + } + } catch (Throwable ignored) { + + } + + AbstractSpan span = ContextManager.createExitSpan(Constants.RETRIEVAL + "/" + dataSourceId, dataSourceId); + + SpanLayer.asGenAI(span); + span.setComponent(ComponentsDefine.SPRING_AI); + Tags.GEN_AI_OPERATION_NAME.set(span, Constants.RETRIEVAL); + Tags.GEN_AI_DATA_SOURCE_ID.set(span, dataSourceId); + String model = resolveEmbeddingModelName(objInst); + if (StringUtils.hasText(model)) { + Tags.GEN_AI_REQUEST_MODEL.set(span, model); + } + + if (request != null) { + Tags.GEN_AI_TOP_K.set(span, String.valueOf(request.getTopK())); + if (StringUtils.hasText(request.getQuery())) { + Tags.GEN_AI_RETRIEVAL_QUERY_TEXT.set(span, request.getQuery()); + } + } + } + + @Override + public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, + Object ret) throws Throwable { + if (!ContextManager.isActive()) { + return ret; + } + try { + if (ret instanceof List<?>) { + Tags.GEN_AI_RETRIEVAL_DOCUMENTS.set(ContextManager.activeSpan(), toDocumentsJson((List<?>) ret)); Review Comment: `gen_ai.retrieval.documents` is likewise emitted unconditionally, unlike `COLLECT_OUTPUT_MESSAGES` which gates analogous response content (default `false`). Lower risk than the query text since only `id` + `score` are serialized (not document bodies), but for consistency consider a `COLLECT_RETRIEVAL_DOCUMENTS = false` flag. Note the JSON grows with `topK` and has no size cap. ########## apm-sniffer/apm-sdk-plugin/spring-plugins/spring-ai-1.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/spring/ai/v1/AbstractObservationVectorStoreInterceptor.java: ########## @@ -0,0 +1,162 @@ +/* + * 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.plugin.spring.ai.v1; + +import org.apache.skywalking.apm.agent.core.context.ContextManager; +import org.apache.skywalking.apm.agent.core.context.tag.Tags; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; +import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult; +import org.apache.skywalking.apm.agent.core.util.GsonUtil; +import org.apache.skywalking.apm.network.trace.component.ComponentsDefine; +import org.apache.skywalking.apm.plugin.spring.ai.v1.contant.Constants; +import org.springframework.ai.document.Document; +import org.springframework.ai.vectorstore.SearchRequest; +import org.springframework.ai.vectorstore.observation.AbstractObservationVectorStore; +import org.springframework.ai.vectorstore.observation.VectorStoreObservationContext; +import org.springframework.util.StringUtils; + +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +public class AbstractObservationVectorStoreInterceptor implements InstanceMethodsAroundInterceptor { + + @Override + public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, + MethodInterceptResult result) throws Throwable { + SearchRequest request = (SearchRequest) allArguments[0]; + String dataSourceId = objInst.getClass().getSimpleName(); + + try { + VectorStoreObservationContext context = + createObservationContext(objInst, request); + + String resolved = + resolveDataSourceId(context, objInst); + + if (StringUtils.hasText(resolved)) { + dataSourceId = resolved; + } + } catch (Throwable ignored) { + + } + + AbstractSpan span = ContextManager.createExitSpan(Constants.RETRIEVAL + "/" + dataSourceId, dataSourceId); + + SpanLayer.asGenAI(span); + span.setComponent(ComponentsDefine.SPRING_AI); + Tags.GEN_AI_OPERATION_NAME.set(span, Constants.RETRIEVAL); + Tags.GEN_AI_DATA_SOURCE_ID.set(span, dataSourceId); + String model = resolveEmbeddingModelName(objInst); + if (StringUtils.hasText(model)) { + Tags.GEN_AI_REQUEST_MODEL.set(span, model); + } + + if (request != null) { + Tags.GEN_AI_TOP_K.set(span, String.valueOf(request.getTopK())); + if (StringUtils.hasText(request.getQuery())) { + Tags.GEN_AI_RETRIEVAL_QUERY_TEXT.set(span, request.getQuery()); + } + } + } + + @Override + public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, + Object ret) throws Throwable { + if (!ContextManager.isActive()) { + return ret; + } + try { + if (ret instanceof List<?>) { + Tags.GEN_AI_RETRIEVAL_DOCUMENTS.set(ContextManager.activeSpan(), toDocumentsJson((List<?>) ret)); + } + } finally { + ContextManager.stopSpan(); + } + return ret; + } + + @Override + public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, + Class<?>[] argumentsTypes, Throwable t) { + if (ContextManager.isActive()) { + ContextManager.activeSpan().log(t); Review Comment: This PR introduces `ErrorTypeResolver.setErrorType()` and wires it into the chat interceptors' error paths, but the retrieval span here only calls `log(t)` (and `EmbeddingModelInterceptor.handleMethodException` is empty). So the retrieval span — the feature this PR adds — is the one that doesn't get `error.type`. Suggest calling `ErrorTypeResolver.setErrorType(span, t)` here too, for consistency. ########## apm-sniffer/apm-sdk-plugin/spring-plugins/spring-ai-1.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/spring/ai/v1/common/ErrorTypeResolver.java: ########## @@ -0,0 +1,81 @@ +/* + * 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.plugin.spring.ai.v1.common; + +import org.apache.skywalking.apm.agent.core.context.tag.AbstractTag; +import org.apache.skywalking.apm.agent.core.context.tag.Tags; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; + +import javax.net.ssl.SSLHandshakeException; +import java.net.SocketTimeoutException; +import java.security.cert.CertPathValidatorException; +import java.security.cert.CertificateException; +import java.util.concurrent.TimeoutException; + +public final class ErrorTypeResolver { + + private static final AbstractTag<String> ERROR_TYPE = Tags.ofKey("error.type"); + private static final String TIMEOUT = "timeout"; + private static final String SERVER_CERTIFICATE_INVALID = "server_certificate_invalid"; + private static final String OTHER = "_OTHER"; Review Comment: `OTHER = "_OTHER"` is never referenced — `resolve()` falls back to `throwable.getClass().getName()` instead. Either remove the constant or return it as the catch-all. Worth noting `_OTHER` is the OpenTelemetry `error.type` convention for unclassified errors, so returning the raw class name rather than `_OTHER` may be an intentional divergence — just confirm which you want. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
