TomcatInvokeInterceptor doesn't include HTTP method in endpoint name

[BFergerson]

Why do you submit this issue?

• Feature or performance improvement

---

Requirement or improvement

You can use "Group Parameterized Endpoints" to collapse endpoint /users/10 and /users/100 but it doesn't differentiate between HTTP method types so doing a {GET}/users/10 looks the same as {DELETE}/users/10.

I've created a patch that adds the HTTP method type before the URL but I'm curious if anyone knows whether the dynamic endpoint can also be determined. That's the /users/{id} part that's defined in @RequestMapping. My guess is you can't since Tomcat sits a layer above SpringMVC. If that's the case, could you update an entry span's operation name? I ask because I would rather auto-detect this information than create the necessary endpoint-name-grouping.yml file. I like that that file exists but I think it's best to consider it as a last result. The idea would be to let Tomcat set the operation name and if a better operation name can be determined by SpringMVC, it would override the original operation name.

If neither method works, is there a way to implement something like USE_QUALIFIED_NAME_AS_ENDPOINT_NAME like SpringMVC, or is that again an issue because Tomcat sits above SpringMVC?

[wu-sheng]

Hi, we are going to recommend post:/uri globally. Ans support OpenAPI spec to group. @wankai123 is working on this.

[wu-sheng]

I would like to say, it should be in defaul as http method + uri, and reuse config globally.

[BFergerson]

@wu-sheng, always so quick with the responses, thanks. I'm glad we're moving to standard. I remember bringing up that I wasn't sure which style made the most sense.

What do you think about my comments on updating the operation name though? Even using the OpenAPI spec there will be the issue of not being able to determine the dynamic endpoint part. The {id} part, since that's defined in the SpringMVC layer. From my understanding, what you're saying will only result in post:/users/10 and delete:/users/100 and I'd like to get it fully to post:/users/{id} and delete:/users/{id}.

[wu-sheng]

The good part of openAPI spec, this kind of id thing is defined there. Once users import their spec files, we could generate grouping rulea accordingly.
But we need to always include http method in a consistent way,meaning remove the <>, but add : between method and uri.

[BFergerson]

If I understand you correctly, you're indicating that dynamic endpoints will no longer be auto-detected and must be manually specified. As in, the goal is replacing/enhancing endpoint-name-grouping.yml with an additional OpenAPI specification file.

If that's the case, why is that considered a better solution than auto-detection? The documentation states "In most cases, the endpoint should be detected automatically through the language agents" and I would think that would be the most desirable default state. With that approach, it would only be those that wish for more fine-grained or accurate detection that would need the OpenAPI specification file.

Perhaps I'm just misunderstanding. I'll admit I have only a passing knowledge of OpenAPI.

[wu-sheng]

Oh, it is not a replacement, instead, it is an addon or enhancement. As in pure Tomcat case, we can't identify this like we did in SpringMVC, also simila case in other language agents, or gateway scenarios.

[wu-sheng]

If tomcat works with SpringMVC, span operation would be overrided.

[wu-sheng]

@BFergerson Codes like RequestMappingMethodInterceptor#getAcceptedMethodTypes, I think we need some adjustment, to follow the same name pattern like this

skywalking/oap-server/server-receiver-plugin/envoy-metrics-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/envoy/als/LogEntry2MetricsAdapter.java

Lines 129 to 136 in efcb396

	protected String endpoint() {
	if (!entry.hasRequest()) {
	return "/";
	}
	final HTTPRequestProperties request = entry.getRequest();
	final String method = request.getRequestMethod().name();
	return method + ":" + request.getPath();
	}

Meaning, remove < and >.

Also, I think we need to discuss whether we should keep runtime HTTP method rather than declaration level methods, only the latter could be multiple values.

[wu-sheng]

HTTP Method in runtime + : + uri would be recommended for all HTTP/1.x endpoint name, unless using method signature as endpoint name.

[wu-sheng]

OpenAPI has been supported.