mujx / hakatime

Wakatime server implementation & analytics dashboard
https://hakatime.mtx-dev.xyz
The Unlicense
631 stars 47 forks source link

Error sending data from Atom plugin v9.0.2 and v10.0.0 #16

Closed agrrh closed 3 years ago

agrrh commented 3 years ago

So I saw code 400 while sending data to precious hakatime from my Atom plugin. Upgrading from v9.0.2 to v10.0.0 makes no difference.

The error was following:

{"now": "2021/01/24 02:32:33 +0300", "version": "13.0.7", "plugin": "atom-wakatime/10.0.0", "time": 1611444753.4426773, "caller": "wakatime/api.py", "lineno": 334, "is_write": true, "file": "/<redacted-path-to-yml-file>", "level": "ERROR", "message": "{'response_code': 400, 'response_content': '{\"error\":\"Bad Request\",\"message\":\"Error in $[0].lineno: parsing Int64 failed, expected Number, but encountered String\"}'}"}

I checked #11 and applied same patch. Sadly, I wasn't quite realizing what I am doing, so would be awesome if you review it first :smile_cat:

It also likely that I've faced same issue as in #1, #10 and #15, so I decided to add small mention to readme, saying the docker-compose relies on local ./docker directory, so we need to clone repo first, not just copy-paste docker-compose.yml itself.

agrrh commented 3 years ago

Ouch, looks like my editor auto-formatted trailing whitespaces and that's a pain to rewrite the whole thing back :facepalm:

I hope that's makes no difference, but please let me know if that's an issue and I should return those spaces back :pray:

agrrh commented 3 years ago

Fixed commit message

moritz-topp commented 3 years ago

Nice to see this getting fixed! Just saw that all my Atom Sessions were not uploaded :/

But the check failed because it couldn't login to docker registry, how can we fix this?

Would be nice if this PR would be accepted soon :D

agrrh commented 3 years ago

My guess is that docker push process not properly set up for PR jobs, so that possibly would success after PR is merged

williamboman commented 3 years ago

Ouch, looks like my editor auto-formatted trailing whitespaces and that's a pain to rewrite the whole thing back 🤦

Doesn't seem like there's that many changes? A quick git checkout -p origin/master should do it

agrrh commented 3 years ago

@williamboman Wow, that was so simple with --patch flag. I was expecting much more manual job. My thanks, learning something new every day!

mujx commented 3 years ago

Hey @agrrh, thanks a lot for the patch!

I tried the atom plugin and from what I see it actually sends ints as strings which in principle is a bug in the plugin but we can work around it.

The PR as is won't work because we don't handle the SQL schema. We currently insert TEXT instead of the expected INT. We can either cast it at the SQL query we use to insert the data or we can change the schema. Note that the second option we require some data migration.

I'm more inclined to keep the schema as is and use something like CAST(line_no AS INT) when we insert the heartbeat.

Edit: The circleci job is supposed to run on every PR not the travis one. Will try to fix that.

agrrh commented 3 years ago

@mujx Hello and thanks for great tool!

I'm more inclined to keep the schema as is and use something like CAST(line_no AS INT) when we insert the heartbeat.

Of course, let's do it that way :+1: I've rolled things back and would test this idea in next few minutes.

Just as a thought, is it possible that Wakatime plugins could send something more complex, like 123:45 structures that's why they use text? :thinking:

mujx commented 3 years ago

Just as a thought, is it possible that Wakatime plugins could send something more complex, like 123:45 structures that's why they use text? 🤔

Judging from the plugin code (and the python cli), that doesn't seem to be the case. It's just the line number. The column number is a separate field if that's what you meant in your example. But if things like that popup in the future, the we'll have to use text.

https://github.com/wakatime/atom-wakatime/blob/caacd1ba14f0716920096ee7b6ccd09d2666f367/lib/wakatime.js#L310-L313

agrrh commented 3 years ago

@mujx Thank you for assistance, I've tested changes by building custom image. As far as I can see, everything works now.

I also rebased the changes to get rid of rollback commits, leaving only the CAST() idea and small docs update regarding SQL files.

mujx commented 3 years ago

Perfect! Thanks again!

1dotd4 commented 2 years ago

This is wrong and against the specs. The Atom plugin should be fixed instead.

1dotd4 commented 2 years ago

This is also dated Jan 25, while the new wakatime-cli was introduced in Atom in May. Reading the code the Marshal in Go happens to be correct. I'm trying to see where python was doing it wrong.

1dotd4 commented 2 years ago

I think that wakatime in Python was taking arguments as strings and passing them to the constructor that did not check if they were numbers. Thus serializing an heartbeat gave a JSON not conform to the specs.