-
Notifications
You must be signed in to change notification settings - Fork 228
DOC-5424 time series updates #1830
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from a few comments, only one of them actionable, LGTM. I really like the changes. The content is so much more readable now.
. | ||
``` | ||
|
||
## Adding data points |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really should be "Add data points" per the SG. Same for other headings.
I know it's kinda weird (I used gerunds in titles all day long in a previous job).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dwdougherty You're quite right - fixed. Thanks for spotting those.
@@ -46,11 +49,11 @@ The graphs and tables below make these key points: | |||
|
|||
- We've observed a maximum 95% drop in the achievable ops/sec even at 99% out-of-order ingestion. (Again, reducing the chunk size can cut the impact in half.) | |||
|
|||
<img src="images/compressed-overall-ops-sec-vs-out-of-order-percentage.png" alt="compressed-overall-ops-sec-vs-out-of-order-percentage"/> | |||
{{< image filename="/images/timeseries/compressed-overall-ops-sec-vs-out-of-order-percentage.webp" alt="compressed-overall-ops-sec-vs-out-of-order-percentage" >}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because I'm curious... was there an edict to begin using WebP images rather than PNGs? The downside to WebP images is that they don't render in GitHub. In other PRs where graphics are swapped, you could see the change side by side in the PR diff viewer; can't do that with WebP. No axe to grind; just thought I'd ask.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dwdougherty It's just the file size (typically about half the size of the equivalent PNG, and no need to use pngcrush or other tools to optimise). "Speed is our superpower", etc, and we're guided to try to have the fastest everything (website, tools...) so I figured that gradually de-crufting and optimising what we've got in the docs was in order.
linkTitle: Out-of-order/backfill performance | ||
aliases: | ||
- /develop/data-types/timeseries/reference/out-of-order_performance_considerations | ||
- /develop/data-types/timeseries/reference/out-of-order_performance_considerations/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just started seeing aliases like this, with both no trailing slash and with. Are both necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dwdougherty TBH, I don't know, but my recent restructuring PR apparently had dodgy aliases (even though they looked right) and the PR to fix had both slash and no-slash aliases (and was reported to work OK). I don't know why it would make any difference, but I still find Hugo a bit enigmatic :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You and me both. 🫥
You might ask Lior to review. It's up to you. |
|
||
The timestamp for each data point is a 64-bit integer value. This is designed | ||
to support Unix timestamps, measured in milliseconds since the | ||
[Unix epoch](https://en.wikipedia.org/wiki/Unix_time). However, you can interpret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can interpret the timestamps in any way you like
This is not a good idea. For example, look at https://redis.io/docs/latest/develop/data-types/timeseries/configuration/#compaction_policy--ts-compaction-policy, where you can specify bucket durations using specific units. There may be other compatibility issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LiorKogan Thanks - I've replaced any mention of days or other time units with milliseconds.
``` | ||
|
||
You can also add one or more *labels* to a time series when you create it. Labels | ||
are key-value pairs where the value can be a string or a number. You can use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name-value instead of key-value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Values are always strings, even if they represent numbers, so I wouldn't put it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed both.
|
||
## Query data points | ||
|
||
Use [`TS.GET`]({{< relref "commands/ts.get/" >}}) to retrieve the last data point |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe 'latest' instead of 'last'. It is not the last point added, but the point with the highest timestamp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this to "highest timestamp" everywhere it occurs (even "latest" can potentially mean "most recently added").
Thanks @dwdougherty and @LiorKogan for such a quick and thorough review of a big change :-) BTW, the first client code examples for the page are in review and should be added shortly. |
DOC-5424
The changes have actually turned out to be quite extensive. In particular, I've reworked the CLI examples quite a bit (the old ones were short, didn't show any output, and didn't lend themselves to equivalent client code examples). Also, I've deleted the redundant build and deployment instructions and moved the single reference page up a level, with images also re-housed and converted to WebP format.
Code examples are in development, btw, but I thought it would be best to make sure the CLI examples are suitable and complete before finalising the code.