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

Problem with InfluxDB and filedKey type for trade ID #425

Closed
igorjan opened this issue Feb 24, 2021 · 14 comments
Closed

Problem with InfluxDB and filedKey type for trade ID #425

igorjan opened this issue Feb 24, 2021 · 14 comments
Labels

Comments

@igorjan
Copy link

igorjan commented Feb 24, 2021

When inserting Trades into influxDB the ID field is of type float for Binance and Coinbase while for Poloniex is type string.

When the program is runing it seems to be doing OK insetring data for all three exchanges but periodically it will produce this error.

2021-02-24 14:17:08,143 : ERROR : POST to http://localhost:8086/write?db=CryptoDataV2 failed: 400 - {"error":"partial write: field type conflict: input field "id" on measurement "trades-POLONIEX" is type float, already exists as type string dropped=1"}

To Reproduce
f.add_feed(Poloniex(subscription={'TUSD-USDT'], }, callbacks={TRADES: TradeInflux(db_addr, db = db_name , numeric_type=float )}))

Expected behavior
Trade ID type should be consistent for all exchanges.
Dropping values because of wrong ID type should be avoided.

Operating System:

  • rpi4 linux
@igorjan igorjan added the bug label Feb 24, 2021
@olibre
Copy link
Contributor

olibre commented Mar 9, 2021

Hi @igorjan
I got a similar issue. If you help me I can write another InfluxDB back-end much cleaner.
I would need you test my Git repo and update the documentation. If you validate my improvement, we will together pull a request. Are you motivated?

@bmoscon
Copy link
Owner

bmoscon commented Mar 9, 2021

@olibre I'll test it if you open a PR

@igorjan
Copy link
Author

igorjan commented Mar 9, 2021

Hi @olibre I am happy to test and help with the documentation. Let me know when you have something to test.

@olibre
Copy link
Contributor

olibre commented Mar 9, 2021

All right @igorjan.
I will work for a couple of hours tomorrow on my Git repo.
I do not plan to change the current InfluxDB backend for backward compatibility reasons,
but to add a new one targeting InfluxDB v2. I will also add cache optimization.
Then I will ask you to test it and to help improving code and documentation.
Is it still OK for you @igorjan?

@igorjan
Copy link
Author

igorjan commented Mar 10, 2021

@olibre Yes this sounds good to me.

@olibre
Copy link
Contributor

olibre commented Mar 15, 2021

Hi @igorjan
Sorry I have not yet found enough time for the moment, but do not worry, I will find some time soon…

@bmoscon
Copy link
Owner

bmoscon commented Apr 3, 2021

@olibre - in case you don't have time, can you tell me your planned changes? I'm reworking the influx code now so I can probably do them unless you have already started

@bmoscon
Copy link
Owner

bmoscon commented Apr 6, 2021

@igorjan - I believe this has been fixed - can you test with the latest code from github and let me know? I rewrote a large part of the influx backend, and all trade ids should be strings now

@bmoscon
Copy link
Owner

bmoscon commented Apr 10, 2021

going to close this - I think its fixed, if someone wants to chime in and refute that we can reopen this

@bmoscon bmoscon closed this as completed Apr 10, 2021
@igorjan
Copy link
Author

igorjan commented Apr 12, 2021

I can confirm the issue has been fixed for my use case.

@CommanderJax
Copy link

This change doesn't appear to be backwards compatible, after upgrading cryptofeed there's now a discrepancy between old and new filed types and to the best of my knowledge influx doesn't offer support to change the old field types from float to string. Any chance there can be an option to cast string to floats?

@bmoscon
Copy link
Owner

bmoscon commented Apr 24, 2021

the new version of influx doesnt work well with strings, at least not from what i've seen. it also makes the code unnecessarily complicated and I'm having a hard time finding a reason to justify that.

@bmoscon
Copy link
Owner

bmoscon commented Apr 24, 2021

and yes, the change is not backwards compatible.

@olibre
Copy link
Contributor

olibre commented May 8, 2021

Hi @bmoscon
I am very very busy and did not yet get time to put back my hands on Cryptofeed for four months!
I have no idea when I will be able to resume my WIP tasks on Cryptofeed...
Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants