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

Add HTTP endpoint /api/echo to query-frontend #714

Merged
merged 1 commit into from May 28, 2021

Conversation

kvrhdn
Copy link
Member

@kvrhdn kvrhdn commented May 27, 2021

What this PR does:
Adds a new endpoint /api/echo that always returns with 200 and a body echo.

Which issue(s) this PR fixes:
Closes #624

Checklist

  • Tests updated
  • Documentation added --> should be captured in Document all APIs #428
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -274,3 +276,13 @@ func (t *App) setupModuleManager() error {
func queryEndpoint(cfg *Config) string {
return cfg.HTTPAPIPrefix + "/api/traces/{traceID}"
}

Copy link
Member

Choose a reason for hiding this comment

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

Let me nit you just a bit more :)!

let's make a single method with some really basic tests:

func  applyHTTPPrefix(cfg, path) {
}

Also let's use path.Join instead of string concat.

Thanks for the work, we're almost there!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! I didn't add a unit test for addHTTPAPIPrefix because the function is really small and not very error-prone (feel free to disagree on this!).
If we change this logic at some point, the e2e tests should break.

I did notice whoever that the e2e tests don't configure a prefix... What do you think, should I adapt one of the configs to use a prefix?

The echo endpoint can be used by Grafana to test Tempo whether is reachable. grafana#624
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

lgtm!

@joe-elliott joe-elliott merged commit 57da4f3 into grafana:main May 28, 2021
@kvrhdn kvrhdn deleted the api-echo branch May 28, 2021 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a way for a Grafana datasource to be able to confirm it is correctly configured for Tempo
3 participants