feat: Add path parameter rendering feature

[jagerzhang]

Description

In the current version, path parameters are not rendered properly in the span name (e.g., GET /api/{userId}), leading to confusion for users who expect to see the relevant path information. This issue particularly affects scenarios where dynamic paths are generated.

To address this, a new optional boolean parameter, render_path_parameters, has been added to enable or disable the rendering of path parameters in the span name:

otel_fastapi.FastAPIInstrumentor().instrument_app(app, render_path_parameters=True)

With this change, the spanName GET /api/{userId} will be recorded as GET /api/123456.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Unit tests have been added to ensure that path parameters render correctly in different scenarios.

• TestFastAPIRenderPathParameters in ’tests/test_fastapi_instrumentation_wrapped.py`

Does This PR Require a Core Repo Change?

• Yes. - Link to PR:
• No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

• Followed the style guidelines of this project
• Changelogs have been updated
• Unit tests have been added
• Documentation has been updated

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jagerzhang / name: Jager (2573b91, f123264, 92c4ef6, 20d9c23, e9caecc, 80f6613, 6aa9e8d, 775e049, 9022641, 8c9eb27, 8220231, 1228564, 8ebc857, aa3ba8a, de4f3d7, 4c36774, 339c635)
• ✅ login: heidi229 (c386b99)
• ✅ login: xrmx / name: Riccardo Magliocchetti (e5529d8, ce1f0b1)

[emdneto]

Thanks for the PR.

Some specs guidance:

HTTP span names SHOULD be {method} {target} if there is a (low-cardinality) target available. The {target} SHOULD be http.route for HTTP Server spans. In that case, http.route is the the matched route, that is, the path template in the format used by the respective server framework (e.g., /users/:userID?; {controller}/{action}/{id?}) and the route attribute should have low-cardinality.

So, a genuine question: by providing that support are we not allowing high-cardinality span names?

[jagerzhang]

Thanks for the PR.

Some specs guidance:

HTTP span names SHOULD be {method} {target} if there is a (low-cardinality) target available. The {target} SHOULD be http.route for HTTP Server spans. In that case, http.route is the the matched route, that is, the path template in the format used by the respective server framework (e.g., /users/:userID?; {controller}/{action}/{id?}) and the route attribute should have low-cardinality.

So, a genuine question: by providing that support are we not allowing high-cardinality span names?

Thanks for the CR and your response.

Indeed, there can be issues with high cardinality, which is why this feature is designed to be disabled by default. It can be enabled as needed in specific scenarios, ensuring that it does not affect regular usage.

[jagerzhang]

Hi, Can it be merged now?

[tammy-baylis-swi]

Thanks for contributing and discussing!

There is a breakdown of the same userId example in the table here:

For example, here are potential span names for an endpoint that gets a hypothetical account information:

Span Name	Guidance
get	Too general
get_account/42	Too specific
get_account	Good, and account_id=42 would make a nice Span attribute
get_account/{accountId}	Also good (using the "HTTP route")

My recommendation would be to follow and include userId as a span attribute instead of part of the span name, and prioritize generality over human readability.

[jagerzhang]

Thanks for contributing and discussing!

There is a breakdown of the same userId example in the table here:

For example, here are potential span names for an endpoint that gets a hypothetical account information:

Span Name Guidance
get Too general
get_account/42 Too specific
get_account Good, and account_id=42 would make a nice Span attribute
get_account/{accountId} Also good (using the "HTTP route")
My recommendation would be to follow and include userId as a span attribute instead of part of the span name, and prioritize generality over human readability.

@tammy-baylis-swi Thanks, I Got it.

This may be mainly because our usage scenarios are quite special. For example, we use the following API routing:

/jobs/{biz_name}/{module}/{function}

To call specific business logic, we hope that span name can uniquely identify and track the execution process or results of specific business logic, such as

/jobs/foo_biz/foo_module/foo_func
/jobs/bar_biz/bar_module/bar_func

We hope there are two span records.

Moreover, this PR feature is turned off by default, and can only be enabled by actively setting parameters when needed. I think it does not pose a challenge to the standard, and I hope it can be considered.

Of course, everyone felt that it did not meet the standards, so I had to use callback functions such as request_hook to achieve the goal.

def fastapi_server_request_hook(span, request):
    method = request.get("method")
    path = request.get("path")
    span.update_name(f"{method} {path}")

[tammy-baylis-swi]

Hi @jagerzhang , yes that's an interesting use case. I suggest continuing to use the request hooks, either for setting span name like your example, or for setting one or more custom span attributes that your backend can use to filter. It could be a single "full http route" attribute, or by one each for biz_name, module, function.

[jagerzhang]

Hi @jagerzhang , yes that's an interesting use case. I suggest continuing to use the request hooks, either for setting span name like your example, or for setting one or more custom span attributes that your backend can use to filter. It could be a single "full http route" attribute, or by one each for biz_name, module, function.

ok, I closing this pr now.