-
Notifications
You must be signed in to change notification settings - Fork 72
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
feat(core): use nanosecond precision by default in Point.toLineProtocol() #338
Merged
Merged
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
ffee482
chore: improve PointSettings docs
sranka de2b0d6
feat(core): add convertTimeToNanos fn
sranka 4166f22
feat(core): serialize Point timestamp with nanosecond precision by de…
sranka 12c74fe
feat(ui): test default Point's line protocol serialization
sranka 5ba146d
chore: update changelog
sranka 81c9c8b
chore: improve Point's timestamp docs
sranka d435ec7
feat(core): allow partial point settings
sranka e59ebc8
chore: improve docs about partial settings
sranka 18973b6
chore: proofread doc
sranka File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'm confused by this line. The fact that the timestamp value is a number doesn't mean it's in NS, or does it?
Shouldn't this block be doing the following?
This way it would ensure that no matter if the Point was constructed given a number in seconds or milliseconds, it'll allways pad it with 0s until reaching NS precision.
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.
Passing a number never implied conversion to nanoseconds (even by default). A
Date
instance should be better used, it was/is a preferred choice since it contains an exact timestamp, it does not need further explanation. If you pass a number, its time unit (precision) is external to a point + you also cannot express precise nanoseconds with a number type. This all was the API contract before this PR, it was also documented that way.Padding a number to 19 digits is not compliant with the previous behavior, it is also not something that I would like to introduce. Users can still use a string or a Date value for what they need, but a number value already has a different meaning to what you expect. Maybe that I am too deep in the js client, so that the documentation that seems clear to me is not clear enough :) I would appreciate your suggestions about changing the doc.
https://influxdata.github.io/influxdb-client-js/influxdb-client.point.timestamp.html :
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.
A number value represents time as a count of time units since epoch
That means I can do
Point.timestamp(1620908660)
(seconds) but alsoPoint.timestamp(1620908660999)
(milliseconds) ?Your
convertTimeToNanos
function is supposed to return the timestamp in nanoseconds as a string, right?So how does the following work?
That falls into the block
So the timestamp is simply converted to a string without adding the extra characters needed to be in nanosecond precision.
Or what am I missing here?
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.
...but a number value already has a different meaning to what you expect
this is the part that is not clear, or at least it's not what I would expect.If I say
point.toLineProtocol()
I expect to get the same output format no matter how the point was built. If this is not the case and making it so is not an option because it's a breaking change, I think the documentation fortoLineProtocol()
should make this very clear.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.
A number does not imply a nanosecond precision, it is a fact to accept independently what I or you think about. I am welcome to a documentation change for you to suggest.
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 improved the doc in the Point class in the scope of this PR, I think that it is more explanatory now.
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'm sorry to be a pain but I don't think this is an improvement at all. The added doc. comment says "nanosecond timestamp precision is used when no settings are supplied" which is not true.
Again, if I create a point like this:
Then do
point.toLineProtocol()
, without settings. The new doc states that I'll get nanosecond precision, which I won't because of this block:But either I'm crazy or this is really confusing. It would be nice to have a third opinion... @vlastahajek ?
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 think the confusion comes from the fact that
toLineProtocol
is a public method and you're implying it will always be used with thewriteApi
from this library.My use case was different, I was simply creating points, serializing them with
toLineProtocol()
and sent them to a message broker for Telegraf to consume and write to InfluxDB 2.0, which failed because the timestamp wasn't in nanoseconds.So it's kind of an inconsistency between this library, Telegraf and its influxdb_v2 output plugin.
Having said this, I still think the "toLineProtocol" method should use the same precision in the output no matter how the point was built in the first place.
For now, I'll just create my points with nanosecond strings for my use case.
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 know that telegraf requires nanos and I know your UC since the beginning, I was quite recently sending line protocol data to telegraf through kafka. A developer that uses a number timestamp has to follow the improved documentation of
Point.timestamp()
.point.toLineProtocol()
indeed uses nanosecond precision without settings, it really means that the supplied number is in nanoseconds. For example: 999999999999 number isThe Point implementation is correct in the expectation now, 999999999999 cannot be
2286-11-20T17:46:39.999Z
as you suggest. A number is quite problematic to absorb, that's why I already recommended to use Date instances.