Skip to content
This repository was archived by the owner on Oct 29, 2024. It is now read-only.

add custom timestamp to SeriesHelper #130

Closed
wants to merge 2 commits into from

Conversation

areski
Copy link
Contributor

@areski areski commented Mar 23, 2015

No description provided.

@aviau
Copy link
Collaborator

aviau commented Mar 23, 2015

This is indeed a feature that we need.

I don't think that this is the most intuitive way to do this. Why not just always accept the timestamp parameter when creating new points?

A simple kwargs pop could be done at helper.py#L105.

@areski
Copy link
Contributor Author

areski commented Mar 23, 2015

if you do so there then the 'cls._datapoints' will not be set correctly, this will fail:
cls._datapoints[cls._series_name.format(**kw)].append(cls._type(**kw))

@aviau
Copy link
Collaborator

aviau commented Apr 9, 2015

Sorry for the long delay :(

I have been busy with the ResultSet (which I merged just now). I'll get to this soon!

@areski
Copy link
Contributor Author

areski commented Apr 9, 2015

no worries ;)

@aviau aviau force-pushed the master branch 2 times, most recently from 308a913 to b65e4a4 Compare August 6, 2015 15:20
@gst
Copy link
Contributor

gst commented Aug 25, 2015

@areski
there is a minor flake8 issue :

influxdb/helper.py:147:80: E501 line too long (81 > 79 characters)

are you still around there to complete entirely this PR ? :)

@areski
Copy link
Contributor Author

areski commented Aug 25, 2015

@gst here a fix for the flake8 issue

Can you be more specific about what you mean by completing entirely the PR?

@gst
Copy link
Contributor

gst commented Aug 25, 2015

Can you be more specific about what you mean by completing entirely the PR?

nothing more, or yes if not already done : if you can rebase your branch on top of master and update the pr ?
also @aviau will have to look on this as I don't know how it's still needed ..

@areski
Copy link
Contributor Author

areski commented Aug 25, 2015

Let's see if @aviau wants this in, no point we work on this if it's not needed ;)

@gst
Copy link
Contributor

gst commented Aug 25, 2015

right ;)
let's wait & see..

@aviau
Copy link
Collaborator

aviau commented Aug 25, 2015

Can we have a tests? I'll merge after.

@aviau aviau force-pushed the master branch 2 times, most recently from 4a77161 to cec332f Compare August 28, 2015 19:36
@@ -141,6 +144,13 @@ def _json_body_(cls):
"fields": {},
"tags": {},
}
if 'timestamp' in point.__dict__ \

Choose a reason for hiding this comment

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

getattr(point, 'timestamp', None) would be more pythonic I suppose.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd do :

ts = getattr(point, 'timestamp', 0)
if ts:
    json_point['timestamp'] = ts
...

@aviau
Copy link
Collaborator

aviau commented Nov 17, 2015

@areski Do you want to finish this PR, or should we take over?

Todo:

  • Add tests
  • Change timestamp to time
  • Add a time to each point when time is not specified.

The reason we should add a time when it is not specified is explained by #264. Inserting several points with no time will result in only the last being taken. I am open to other suggestions.

@matt-snider
Copy link
Contributor

@aviau @areski Hi is this PR still going to be developed and merged? :)

This feature would be very useful to us so that we can use a high batch size, while still retaining good time resolution (and not losing any points).

@aviau
Copy link
Collaborator

aviau commented Feb 4, 2016

@matt-snider Would you like to take over?

@matt-snider
Copy link
Contributor

Sorry for the late response.

I will take a look at this and open up another PR when I get some time.

grundleborg added a commit to grundleborg/influxdb-python that referenced this pull request Mar 8, 2016
Point can be specified as either a number of nanoseconds, a python
datetime object (with or without timezone) or a string in ISO datetime
format.

If a time is not specified, the Helper sets the time at the time of
assembling the point fields so that multiple unique points with the same
tags can be committed simultaneously without them failing to add due to
all being assigned the same automatic time by the InfluxDB server.

This fix is based upon the discussion in influxdata#130 but also includes the
outstanding items for it to be merged. I'm happy to receive suggestions
for further ways to add test coverage to this change.

This also fixes influxdata#264 and fixes influxdata#259.
grundleborg added a commit to grundleborg/influxdb-python that referenced this pull request Mar 8, 2016
Point can be specified as either a number of nanoseconds, a python
datetime object (with or without timezone) or a string in ISO datetime
format.

If a time is not specified, the Helper sets the time at the time of
assembling the point fields so that multiple unique points with the same
tags can be committed simultaneously without them failing to add due to
all being assigned the same automatic time by the InfluxDB server.

This fix is based upon the discussion in influxdata#130 but also includes the
outstanding items for it to be merged. I'm happy to receive suggestions
for further ways to add test coverage to this change.

This also fixes influxdata#264 and fixes influxdata#259.
grundleborg added a commit to grundleborg/influxdb-python that referenced this pull request Mar 8, 2016
Point can be specified as either a number of nanoseconds, a python
datetime object (with or without timezone) or a string in ISO datetime
format.

If a time is not specified, the Helper sets the time at the time of
assembling the point fields so that multiple unique points with the same
tags can be committed simultaneously without them failing to add due to
all being assigned the same automatic time by the InfluxDB server.

This fix is based upon the discussion in influxdata#130 but also includes the
outstanding items for it to be merged. I'm happy to receive suggestions
for further ways to add test coverage to this change.

This also fixes influxdata#264 and fixes influxdata#259.
@aviau
Copy link
Collaborator

aviau commented Jun 9, 2016

I'll close this as @matt-snider's PR was merged

@aviau aviau closed this Jun 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants