Add Freeform tempo toolset #941
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds Grafana Tempo integration upgrades: new config models, a retry-enabled Tempo API client, a refactored toolset replacing legacy functions, CLI for Tempo operations, utilities for durations/step sizing, documentation updates, and extensive tests. Removes the old tempo_api module. Minor bugfix in trace parsing. Test fixtures updated for newer Tempo and enriched OTEL resources. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as Tempo CLI
participant API as GrafanaTempoAPI
participant Tempo as Grafana Tempo
User->>CLI: Command (e.g., search-query)
CLI->>API: Initialize (url, api_key, datasource_uid, use_post)
CLI->>API: search_traces_by_query(q, start, end, limit, spss)
API->>Tempo: HTTP GET/POST /api/search?q=...&start=...&end=...
alt 2xx
Tempo-->>API: JSON
API-->>CLI: Result dict
CLI-->>User: Print JSON (pretty/compact)
else HTTP error
Tempo-->>API: 4xx/5xx
API-->>CLI: TempoAPIError(status_code, url, response_text)
CLI-->>User: Render error and exit 1
end
sequenceDiagram
autonumber
participant Tool as GrafanaTempoToolset Tool
participant Pre as prerequisites_callable
participant API as GrafanaTempoAPI
participant Tempo as Grafana Tempo
Tool->>Pre: prerequisites_callable(config)
Pre->>API: query_echo_endpoint()
API->>Tempo: /api/echo (GET/POST)
alt 200 OK
Tempo-->>API: OK
API-->>Pre: True
Pre-->>Tool: OK to run
else Error/timeout
Tempo-->>API: Error
API-->>Pre: False
Pre-->>Tool: Fail with message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (17)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| try: | ||
| import json | ||
|
|
||
| error_data = json.loads(response_text) |
There was a problem hiding this comment.
Any specific reason to parse this and not just always return response_text? Is there something big we are trying to filter out of the error message in the other fields?
| """Python wrapper for Grafana Tempo REST API. | ||
| This class provides a clean interface to all Tempo API endpoints, | ||
| supporting both GET and POST methods based on configuration. |
There was a problem hiding this comment.
Could we remove the POST support? It seems like a mistake. (There might be specific endpoints that require POST but having this as a global flag doesn't make sense.)
| endpoint: str, | ||
| params: Optional[Dict[str, Any]] = None, | ||
| path_params: Optional[Dict[str, str]] = None, | ||
| timeout: int = 30, |
There was a problem hiding this comment.
I would make this dramatically shorter or remove it altogether. (Why do we need retries at all?) With 30s per timeout * 3 retries, a single call to tempo could take 90s seconds.
| return make_request() | ||
| except requests.exceptions.HTTPError as e: | ||
| # Extract detailed error message from response | ||
| response = e.response |
There was a problem hiding this comment.
See comments in the exception class. Could all this code could be much simpler. Something like raise Exception(f"{e.response}")?
| and getattr(e, "response", None) is not None | ||
| and e.response.status_code < 500, | ||
| ) | ||
| def make_request(): |
There was a problem hiding this comment.
Use of the inner function here seems uncessary. We could just post the code directly in try/except - no need to define a function.
| @@ -0,0 +1,419 @@ | |||
| #!/usr/bin/env python3 | |||
| """ | |||
| @@ -0,0 +1,162 @@ | |||
| # Tempo CLI - Grafana Tempo Command Line Interface | |||
| process_timestamps_to_int, | ||
| ) | ||
|
|
||
| TEMPO_LABELS_ADD_PREFIX = load_bool("TEMPO_LABELS_ADD_PREFIX", True) |
| except Exception as e: | ||
| return False, f"Failed to connect to Tempo: {str(e)}" | ||
|
|
||
| def build_k8s_filters( |
There was a problem hiding this comment.
Consider removing this altogether! Its currently only used by the comparative tool. I wonder if we really need it there or could get comparative to work without it.
old pr and comment we can tackle later - #941 --------- Co-authored-by: Nicolas Herment <[email protected]>
No description provided.