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

Allow to change transaction name #99

Closed
wants to merge 2 commits into from
Closed

Conversation

olerapx
Copy link

@olerapx olerapx commented Oct 3, 2023

Add a PHP function to change transaction name from user code.

This is useful if a PHP project uses GraphQL API. In that case, all requests will be routed through a single endpoint (that is, /graphql), and it becomes impossible to group requests based on endpoint parameters, such as in the REST API case

src/request.rs Outdated
@@ -332,3 +332,14 @@ fn finish_request_context(request_id: Option<i64>, status_code: i32) -> crate::R

Ok(())
}

pub fn skywalking_set_request_name(args: &mut [ZVal]) -> phper::Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

operation name is the official term for the span. Why is this called request name? We don't have this concept.

Copy link
Author

Choose a reason for hiding this comment

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

Renamed to skywalking_set_operation_name

@wu-sheng wu-sheng requested a review from jmjoy October 3, 2023 13:27
@wu-sheng wu-sheng added the enhancement New feature or request label Oct 3, 2023
@heyanlong
Copy link
Member

@olerapx Therefore, if you want to change the operation_name of /graphql, would it be better to build a GraphQL plug-in? This method is not restricted to GraphQL because its reach is too broad.

@olerapx
Copy link
Author

olerapx commented Oct 3, 2023

@olerapx Therefore, if you want to change the operation_name of /graphql, would it be better to build a GraphQL plug-in? This method is not restricted to GraphQL because its reach is too broad.

That would involve parsing the incoming GraphQL query, which feels out of scope for the project. I can, of course, use something like https://docs.rs/graphql-parser/latest/graphql_parser/ and extract operation name from the query, but I think it's more appropriate to leave for the usercode to handle.

The function can be applied not only for GraphQL, sure. That's the point: if the currently provided mechanism of constructing the operation name is not suitable for any particular project, it can be customized from PHP without the need of modifying the skywalking-php further :)

The idea is to make something similar that New Relic or Atatus provide: both services can detect operation name and give the ability to set it manually.

@wu-sheng
Copy link
Member

wu-sheng commented Oct 3, 2023

The side effect of exposing and using this API in your app code, would make your app can't run w/o agent codes. Such as when you face performance impact of extra payload or/and in some circumstances.

@olerapx
Copy link
Author

olerapx commented Oct 3, 2023

The side effect of exposing and using this API in your app code, would make your app can't run w/o agent codes. Such as when you face performance impact of extra payload or/and in some circumstances.

That can be mitigated with a simple if (extension_loaded('skywalking_agent')) check in PHP code. Speaking from NewRelic/Atatus usage experience, it's basically the same with those APMs.

I think, it should just be mentioned somewhere in the docs.

@wu-sheng
Copy link
Member

wu-sheng commented Oct 3, 2023

Generally, if you refer to our Java agent implementation(go agent are going to have this as well), https://skywalking.apache.org/docs/skywalking-java/next/en/setup/service-agent/java-agent/application-toolkit-tracer/, we recommend you to build a toolkit lib with empty implementation and make sure they are harmless.

Your codes should always use the toolkit lib without concerns, and the agent would instrument those APIs when the agent installed.

This is better to add many IF/ELSE in your app codes.

@olerapx
Copy link
Author

olerapx commented Oct 3, 2023

Do you mean creating a PHP library wrapping the skywalking_set_request_name (and, potentially, other) calls with extension_enabled check?
I fully agree that's better than adding if/else in code everytime :)

@wu-sheng
Copy link
Member

wu-sheng commented Oct 4, 2023

Do you mean creating a PHP library wrapping the skywalking_set_request_name (and, potentially, other) calls with extension_enabled check?

You could check things in Java, https://github.com/apache/skywalking-java/blob/main/apm-application-toolkit/apm-toolkit-trace/src/main/java/org/apache/skywalking/apm/toolkit/trace/ActiveSpan.java
All these methods in the lib are empty, so, your app codes could use them at any place you like.
When the extension(PHP agent) is installed, these methods would be enhanced by the agent, and then, the interceptor of these methods would call the original methods. Through this way, you actually don't need any if/else. You could install the agent or uninstall the agent at any time.

@jmjoy
Copy link
Member

jmjoy commented Oct 4, 2023

It is not recommended to use extension functions, as discussed above, when skywalking_agent extension is not loaded, it will result in fatal errors. My suggestion is to use environment variables or other system variables, such as putenv in user codes, and then obtain the environment variables in the extension for further processing.

@wu-sheng
Copy link
Member

wu-sheng commented Oct 4, 2023

It is not recommended to use extension functions, as discussed above, when skywalking_agent extension is not loaded, it will result in fatal errors. My suggestion is to use environment variables or other system variables, such as putenv in user codes, and then obtain the environment variables in the extension for further processing.

setOperationName is a runtime operation per request context, how could env work for this case?

@jmjoy
Copy link
Member

jmjoy commented Oct 7, 2023

It is not recommended to use extension functions, as discussed above, when skywalking_agent extension is not loaded, it will result in fatal errors. My suggestion is to use environment variables or other system variables, such as putenv in user codes, and then obtain the environment variables in the extension for further processing.

setOperationName is a runtime operation per request context, how could env work for this case?

For php-fpm, env is request context.

@wu-sheng
Copy link
Member

wu-sheng commented Oct 7, 2023

OK, if that is the case, it should be good to use it as a bridge between app codes and agent kernel.

@wu-sheng
Copy link
Member

Close as no active for months.

@wu-sheng wu-sheng closed this Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants