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 support for tsdb endpoint #773

Merged
merged 1 commit into from Jun 24, 2020

Conversation

HimaVarsha94
Copy link
Contributor

@HimaVarsha94 HimaVarsha94 commented Jun 22, 2020

Two primary updates:

  • api/prometheus/v1/api.go: Add support for tsdb endpoint
  • api/prometheus/v1/api_test.go: Add test case for tsdb endpoint

Signed-off-by: Hima Varsha hdureddy@apple.com

@HimaVarsha94
Copy link
Contributor Author

HimaVarsha94 commented Jun 22, 2020

cc @beorn7

@HimaVarsha94 HimaVarsha94 marked this pull request as ready for review Jun 22, 2020
@beorn7 beorn7 requested a review from bwplotka Jun 23, 2020
@beorn7
Copy link
Member

beorn7 commented Jun 23, 2020

Thanks @HimaVarsha94 .

@lilic @bwplotka could you have a look? You know this stuff better than me.

Copy link
Member

@bwplotka bwplotka left a comment

Looks awesome, just small naming suggestion. Thanks! 🤗

@@ -404,6 +407,20 @@ type queryResult struct {
v model.Value
}

// TSDBResult contains the result from querying the tsdb endpoint
type TSDBResult struct {
Copy link
Member

@bwplotka bwplotka Jun 23, 2020

Choose a reason for hiding this comment

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

I am maybe bias, but I wish we could just import https://github.com/prometheus/prometheus/blob/master/web/api/v1/api.go#L1172

This is fine though I guess - to avoid importing prometheus.

Copy link
Contributor

@lilic lilic Jun 24, 2020

Choose a reason for hiding this comment

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

It might to avoid cyclic dependencies? In any case, I would stick to what already is standard practice here but we could open issue for this to discuss?

api/prometheus/v1/api.go Outdated Show resolved Hide resolved
api/prometheus/v1/api.go Outdated Show resolved Hide resolved
Signed-off-by: Hima Varsha <hdureddy@apple.com>
lilic
lilic approved these changes Jun 24, 2020
Copy link
Contributor

@lilic lilic left a comment

Thanks! lgtm 🎉

@@ -404,6 +407,20 @@ type queryResult struct {
v model.Value
}

// TSDBResult contains the result from querying the tsdb endpoint
type TSDBResult struct {
Copy link
Contributor

@lilic lilic Jun 24, 2020

Choose a reason for hiding this comment

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

It might to avoid cyclic dependencies? In any case, I would stick to what already is standard practice here but we could open issue for this to discuss?

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.

None yet

4 participants