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

Deleting a logstream should delete its stats. #419

Merged

Conversation

davidrhp
Copy link
Contributor

@davidrhp davidrhp commented May 22, 2023

Fixes #412 .

Description

The intention is to fix the bug mentioned in the issue, where get_stats reported the last recorded data of a logstream, even though it has already been deleted. Since the logstream has already been deleted, no stats data should be available.

Returning a 404

Currently, the stats::get_current_stats also returns Stats for logstreams that do not exist and fills the struct with 0 values. This leads to callers not actually knowing, whether stats for this stream actually exist. While the signature actually uses an Option<Stats>, the None variant is only returned, if the underlying prometheus function returns an Err. According to the docs, this should only happen, if the number of labels don't match.

Since the underlying MetricVecs containing the stats, don't seem to have a method to check if the passed in labels already exist, it seems to be better to check at the caller's side whether a stream exists using metadata::STREAM_INFO.stream_exists(..). If it does, check the stored metrics (stats), if not, return a 404.

Deleting stats when a stream is deleted

I added a function in the stats module to delete the metrics for the deleted logstream, which is then called in the delete logstream handler.

Since the prometheus::vec::MetricVec::get_metric_with_label_values and prometheus::vec::MetricVec::remove_label_values explicitly warn of passing multiple labels into the respective function, I extracted the label creation into their own functions, so both the stats::get_current_stats and the new stats::delete_stats always use the same labels.

Warning
Previously, the get_stats handler returned a 200, even for logstreams that have never been created. Now, it returns a 404 for logstreams that do not exist.


This PR has:

  • been tested to ensure log ingestion and log query works.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added documentation for new or modified features or behaviors.

@nitisht nitisht requested a review from trueleo May 22, 2023 09:41
…Deleting a logstream also deletes its corresponding stats. (parseablehq#412)
@davidrhp davidrhp force-pushed the bug/delete_logstream_stats_still_visible branch from 7cc0c36 to e811483 Compare May 22, 2023 09:55
@davidrhp
Copy link
Contributor Author

@trueleo I am ready with this PR, however, I am not sure about this point:

"[] added documentation for new or modified features or behaviors."

This PR will change the Stream Storage Stats API by returning 404s for logstreams that do not exists. Previously, it has returned a 200, with zeros for the respective metrics.

I could not find a section in the docs where this behavior was documented. If it is not, I would mark this PR as ready.

@nitisht nitisht marked this pull request as ready for review May 22, 2023 14:07
@nitisht nitisht marked this pull request as draft May 22, 2023 14:07
@nitisht
Copy link
Member

nitisht commented May 22, 2023

Thanks for the PR @davidrhp .

"[] added documentation for new or modified features or behaviors."

The documentation is unfortunately not yet open source and you're correct in that it doesn't cover the response. So I think we're fine on this point. Please mark this ready and we can merge it soon

@davidrhp davidrhp marked this pull request as ready for review May 22, 2023 14:49
@davidrhp
Copy link
Contributor Author

@nitisht Thanks for the quick response. I marked the PR accordingly.

@nitisht nitisht merged commit 634129b into parseablehq:main May 22, 2023
@davidrhp davidrhp deleted the bug/delete_logstream_stats_still_visible branch May 23, 2023 07:42
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.

bug: after deleting a log stream, still able to get the stats for that logstream
3 participants