[ENHANCEMENT] Tempo: Use Query V2 API and surface API (warning) messages#726
[ENHANCEMENT] Tempo: Use Query V2 API and surface API (warning) messages#726andreasgerstmayr wants to merge 2 commits into
Conversation
Existing datasources may only have /api/traces in their allowed endpoints list, therefore fall back to the v1 API on 403 errors for backwards compatibility. Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
8b5c908 to
b3b20b1
Compare
| } catch (e) { | ||
| // Existing datasources may only have /api/traces in their allowed endpoints list. | ||
| // Fall back to the v1 API on 403 errors for backwards compatibility. | ||
| if (e instanceof Error && 'status' in e && e.status === 403) { |
There was a problem hiding this comment.
This fallback triggers on any 403, so genuine auth failures will also be caught here. Would it be better to allow the client to be configured to use a specific version?
There was a problem hiding this comment.
I would like to avoid the additional complexity of introducing a data source configuration setting for this setting. Potentially in the future there will be a V2 of the search endpoint, then we'll end up with two boolean configs "enable query v2" and "enable search v2".
For the UI path, I could enable the new API by default, and keep existing datasources as-is. But when the datasource is provisioned via the operator, I also would like to enable the new API by default, however I cannot differentiate between new datasources (where this config property is unset) and existing datasources (where this config property is also unset). Unless we create some upgrade/migration path (enable v1 API for existing datasources), but that again adds complexity.
Ideally I'd also remove the fallback-to-v1-on-403-error workaround after some time (a year?).
What do you think?
There was a problem hiding this comment.
I think mixing the 2 versions might lead to issues other than the one with the 403. I'd say is better to default to the new version, and if existing users still want v1 then we should allow something like query: 'v1' so they can use the v1 client, then we can mark this one as a Breaking change. WDYT @Nexucis ?
There was a problem hiding this comment.
The Query V2 API was added in Tempo v2.6.0 (released Sep 3, 2024), I don't think there's a good reason to keep supporting the old v1 API.
afaics the only issue is the allowed endpoints list, which will prevent connections to the new API for existing Tempo datasources. But if we mark it as a breaking change, I guess it should be ok (the fix is simple, to update the list of allowed endpoints in the datasource).
There was a problem hiding this comment.
In that case we can remove the catch code falling back to v1 and create a new minor version for the plugin, marking the breaking change and adding in the PR small documentation on how to "migrate".
Co-authored-by: Gabriel Bernal <gabrielbernalp@gmail.com> Signed-off-by: Andreas Gerstmayr <andreas@gerstmayr.me>
Description
Use Query V2 API and surface API (warning) messages.
Existing datasources may only have /api/traces in their allowed endpoints list, therefore fall back to the v1 API on 403 errors for backwards compatibility.
Resolves: perses/perses#3961
Screenshots
Example notice:

Checklist
[<catalog_entry>] <commit message>naming convention using one of thefollowing
catalog_entryvalues:FEATURE,ENHANCEMENT,BUGFIX,BREAKINGCHANGE,DOC,IGNORE.UI Changes