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

WIP: Upgrade the InfluxDB module. #1361

Merged
merged 4 commits into from
Dec 15, 2016
Merged

WIP: Upgrade the InfluxDB module. #1361

merged 4 commits into from
Dec 15, 2016

Conversation

tobli
Copy link
Member

@tobli tobli commented Dec 3, 2016

I'm pushing this completely untested. Would love for someone to give it a spin and see if it actually works. Version 5 of the influx module has a completely new API.

@tobli tobli changed the title Upgrade the InfuxDB module. WIP: Upgrade the InfuxDB module. Dec 3, 2016
@tobli tobli mentioned this pull request Dec 3, 2016
5 tasks
@soulgalore soulgalore mentioned this pull request Dec 6, 2016
@eripa
Copy link

eripa commented Dec 6, 2016

I've given this a spin using the docker compose approach, but replacing the graphite container with a custom built container from this commit.

docker run --rm --net=container:sitespeed_influxdb_1 sitespeed-upgrade-influx http://example.com -b firefox --plugins.load influxdb --influxdb.host influxdb --influxdb.database sitespeed_io

It works fine as far as I can see. I'm able send data with the below caveat. I think they are more generic issues for InfluxDB support than issues with this upgrade. Should I add them to #889?

  • I need to manually create the InfluxDB database before starting to send data
  • each metric is sent as a complete series, which means that I cannot break it up on various levels (like it's done using Graphite). This is the same behavior as I see when I run InfluxDB with a Graphite input plugin. I'm not sure how this would work, but I suspect the point data needs to be restructured to be handled properly by InfluxDB

@tobli
Copy link
Member Author

tobli commented Dec 12, 2016

Thanks for the testing @eripa. Given that it seems to work I'll rebase, uncomment the cli documentation, and merge this. With this we should be able to close #889. It'd be lovely if you could file your two suggestions above as two separate issues. Thanks for the feedback!

@tobli tobli changed the title WIP: Upgrade the InfuxDB module. WIP: Upgrade the InfluxDB module. Dec 12, 2016
@soulgalore
Copy link
Member

each metric is sent as a complete series, which means that I cannot break it up on various levels (like it's done using Graphite)

hmm shouldn't we see if we can fix this before we close #889 ?

@eripa
Copy link

eripa commented Dec 13, 2016

Personally I would recommend looking at fixing the metric split issue. Having it the way it is virtually removes the possibility to use variables in dashboards as you will need to create a variable with the complete metric series name.

@soulgalore
Copy link
Member

Also in the compose file the database name is sitespeed and in your example run @eripa it's sitespeed_io, could that be the problem?

@eripa
Copy link

eripa commented Dec 13, 2016

The problem in what issue, @soulgalore? The fact that I needed to create the database manually?

Or do you think it will make any difference having the Influx database be named just sitespeed in regards to the metric split issue?

@soulgalore
Copy link
Member

ah sorry @eripa yes I meant that in your example the database was called sitespeed_io but in the compose file it is sitespeed only, but maybe did your own setup? Maybe it's also better to have a create script instead of specific tutum/influxdb thing. however the other issue seems more important.

@eripa
Copy link

eripa commented Dec 13, 2016

Ah, I see. No worries! Yes I used my own docker-compose file.. And in the container image I uses there doesn't seem to be any "precreate" functionality.

Excerpt for my docker-compose:

  influxdb:
    image: influxdb:alpine
    volumes:
      - ./influxdb-data:/var/lib/influxdb/data
      - ./influxdb-wal:/var/lib/influxdb/wal
      - ./influxdb.conf:/etc/influxdb/influxdb.conf:ro
    restart: always
    command: influxd -config /etc/influxdb/influxdb.conf

@soulgalore
Copy link
Member

Cool, I see now, we should also switch to the official one (and bind to a specific version). About the database: I guess we can use the API as in https://hub.docker.com/_/influxdb/

Also think we should have some default dashboard when we release. We can merge the PR but I think we should have the cli options commented out for now until we have everything fixed.

@eripa
Copy link

eripa commented Dec 13, 2016

Sounds good to me! Should I create a separate issue for the split metric issue?

@soulgalore
Copy link
Member

@eripa yes please and examples if you have :)

@soulgalore soulgalore merged commit e660a77 into master Dec 15, 2016
@soulgalore soulgalore deleted the upgrade-influx branch December 15, 2016 10:37
@aliradd
Copy link

aliradd commented Feb 4, 2017

Hi there,
If you guys like someone to load test the write capability I have a solution ready to go.
I would need a little help to get this plugging up and running but on the load generation side my tool is ready. We can perhaps try to see how much we could push into influx etc....not sure if this is of interest to you?

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.

5 participants