Skip to content

Add MonitoringService API#96

Merged
areina merged 2 commits into
mainfrom
feat/toni/add-monitoring-service
Jun 3, 2025
Merged

Add MonitoringService API#96
areina merged 2 commits into
mainfrom
feat/toni/add-monitoring-service

Conversation

@areina
Copy link
Copy Markdown

@areina areina commented May 29, 2025

No description provided.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 29, 2025

The latest Buf updates on your PR. Results from workflow Pull Request / linting (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedJun 2, 2025, 6:27 PM

@areina areina force-pushed the feat/toni/add-monitoring-service branch from 51e5022 to 15ca46b Compare May 29, 2025 19:10
Comment thread proto/qdrant/cloud/monitoring/v1/monitoring.proto Outdated
@areina areina marked this pull request as ready for review May 30, 2025 08:32
@areina areina requested a review from a team as a code owner May 30, 2025 08:32
optional google.protobuf.Timestamp until = 4;
// Aggregation function to apply to the time series data.
// If omitted, defaults to SUM.
optional Aggregator aggregator = 5;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we specify a resolution, or is this automatically inferred by the span (aka duration)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Step is automatically chosen depending on the metric and the span

Comment thread proto/qdrant/cloud/monitoring/v1/monitoring.proto
Comment thread proto/qdrant/cloud/monitoring/v1/monitoring.proto Outdated
Comment thread proto/qdrant/cloud/monitoring/v1/monitoring.proto Outdated

// IntervalAverage represents the average value of a metric over a specific time interval.
message IntervalAverage {
// TODO which unit do they represent? minutes? Also, can we change them to int?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what about google.protobuf.Duration, so we can do 5min resolution ?
Shouldn't we add this to the request and only provide a single result?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think google.protobuf.Duration makes sense. Currently the response is always including 4 intervals, looking at the code it seems they are (current value, last hour, last 3 hours, last 24 hours).

I guess for current we could set a Duration of 0.

About changing the request and returning only a single result, I think that would require more changes in the UI and not sure if it's worth the investment at this moment. However, I'm open to discuss it.

Comment thread proto/qdrant/cloud/monitoring/v1/monitoring.proto Outdated
Comment thread proto/qdrant/cloud/monitoring/v1/monitoring.proto Outdated
Copy link
Copy Markdown
Contributor

@mikheillomidzeq mikheillomidzeq left a comment

Choose a reason for hiding this comment

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

Left comments

@areina areina force-pushed the feat/toni/add-monitoring-service branch from 15ca46b to 78cd122 Compare June 2, 2025 18:27
Copy link
Copy Markdown
Contributor

@mikheillomidzeq mikheillomidzeq left a comment

Choose a reason for hiding this comment

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

LGTM!

@areina areina merged commit 54d0957 into main Jun 3, 2025
14 checks passed
@areina areina deleted the feat/toni/add-monitoring-service branch June 3, 2025 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants