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

MF-454 - Use message Time field as a time for InfluxDB points #455

Merged
merged 4 commits into from
Nov 18, 2018

Conversation

dborovcanin
Copy link
Collaborator

Signed-off-by: Dusan Borovcanin dusan.borovcanin@mainflux.com

What does this do?

Use time from received message to set time value in InfluxDB.

Which issue(s) does this PR fix/relate to?

This pull request closes #454.

Notes

This pull request slightly modifies the way messages are saved in InfluxDB. Field "Time" is removed and replaced by default "time" column of InfluxDB. Previously, "time" kept information about timestamp when the message is saved in the database, so that info is no longer persistent in InfluxDB. However, this information can be found in service logs if needed. This also means that the size of tag set of the single point is slightly reduced.

Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

As discussed in the issue - tagset is wrong.

This tagset will form huge number of different series. Only channelID and eventually content-type should be in a tagset.

@codecov-io
Copy link

codecov-io commented Nov 13, 2018

Codecov Report

Merging #455 into master will decrease coverage by 0.08%.
The diff coverage is 93.1%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
- Coverage   91.33%   91.25%   -0.09%     
==========================================
  Files          46       46              
  Lines        1834     1840       +6     
==========================================
+ Hits         1675     1679       +4     
- Misses        106      107       +1     
- Partials       53       54       +1
Impacted Files Coverage Δ
writers/influxdb/messages.go 85.91% <100%> (-0.2%) ⬇️
readers/influxdb/messages.go 88.4% <84.61%> (-1.92%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf299a0...5e05b92. Read the comment docs.

drasko
drasko previously approved these changes Nov 13, 2018
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

nmarcetic
nmarcetic previously approved these changes Nov 16, 2018
Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>
Update all Reader and Writer services to use time from the message
instead of time given from the corrseponding Writer service.

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>
Messages time less than 2**28 represent time relative to the current time so Writer used to convert this to the correct value, i.e. msg.Time += time.Now(). However, this step is optional and should really be a part of the app on top of Mainflux or could be introduced with minor changes in Normalizer, Reader or Writer services, so there is no need for this to be supported out of box.

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>
nmarcetic
nmarcetic previously approved these changes Nov 16, 2018
Move all the other Message fields to the field keys.

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

@drasko drasko merged commit 0379eb6 into absmach:master Nov 18, 2018
@dborovcanin dborovcanin deleted the MF-454 branch January 31, 2019 10:50
davide83 pushed a commit to davide83/mainflux that referenced this pull request May 13, 2019
…h#455)

* Use message time as Point time in InfluxDB

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

* Use actual message time

Update all Reader and Writer services to use time from the message
instead of time given from the corrseponding Writer service.

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

* Remove message time check

Messages time less than 2**28 represent time relative to the current time so Writer used to convert this to the correct value, i.e. msg.Time += time.Now(). However, this step is optional and should really be a part of the app on top of Mainflux or could be introduced with minor changes in Normalizer, Reader or Writer services, so there is no need for this to be supported out of box.

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

* Use channel and publisher as tag keys

Move all the other Message fields to the field keys.

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>
manuio pushed a commit that referenced this pull request Oct 12, 2020
* Use message time as Point time in InfluxDB

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

* Use actual message time

Update all Reader and Writer services to use time from the message
instead of time given from the corrseponding Writer service.

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

* Remove message time check

Messages time less than 2**28 represent time relative to the current time so Writer used to convert this to the correct value, i.e. msg.Time += time.Now(). However, this step is optional and should really be a part of the app on top of Mainflux or could be introduced with minor changes in Normalizer, Reader or Writer services, so there is no need for this to be supported out of box.

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>

* Use channel and publisher as tag keys

Move all the other Message fields to the field keys.

Signed-off-by: Dusan Borovcanin <dusan.borovcanin@mainflux.com>
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.

Use message Time field as a time for InfluxDB points
4 participants