Closed kevinelliott closed 2 years ago
Hi,
thanks for the pull request. A few quick comments.
It would be helpful to limit pull requests to one feature per request.
Definitely. Will adjust.
the sats.json tmpfile/rename is intended to be in the same directory to limit race conditions/filesystem churn. What is your rationale for changing that?
Some of us are wanting to use that file in other areas, or are not storing the scripts we are using in the html dir for the live map.
if you work with timestamps representing different time zones I suggest using iso8601 time zone designators (i.e. append Z or +01:00 or similar) to clarify the output
That is precisely the format that is used.
json output currently only applies to -m acars. It may be worth to spend some time coming up with a more general solution that works in more modes. (Not sure yet how that would look like)
I have definitely considered this. The issue is that upon further use and thought it might be that you don't want json for all of the outputs. Seeing human readable for some things (like the status line) is useful in a log or locally displayed, and not necessarily useful as JSON.
What further conflates this is that some output is stderr, some stdout, and appears like intention there is still being fleshed out.
What if we made it apply only to ACARS for the moment, but I follow it up with further enhancements as we hash it out?
@Sec42 Thoughts on my responses above?
- It would be helpful to limit pull requests to one feature per request.
Also, this is all the same feature, with exception to the two liner for the output path.
Haven't had much time for it. a) if the timstamp is properly tagged, i would just use UTC in json and loose the option. b) still not very happy with the tempfile change as is c) planning to take a look if json could be done in a more general way, just hadn't time for it.
a) if the timstamp is properly tagged, i would just use UTC in json and loose the option.
Understood. I know some folks that would like to have their local logs in the existing format, but happy to make it UTC always myself (selfishly, as an aggregator, want it in UTC when received anyway).
b) still not very happy with the tempfile change as is
I can ditch and leave as a local edit I suppose. I can't use the sat map feature at all without it. I don't want to store my scripts in the HTML dir itself (security issue in theory).
c) planning to take a look if json could be done in a more general way, just hadn't time for it.
Agreed, same here. So, can we just merged this in for acars for now and loop back for the rest?
re a) i don't see json as a human readable format. And if you're processing it with any kind of tool, it should be possible to do timezone conversion there. I mean, decide on one format and use that. I don't really care if it's UTC or localtime as long as it is properly tagged.
re b) you can "cd" to the correct directory before running reassembler with the full path - which is what the "example.sh" already does. -- I am not objecting to putting the file in a different directory. I'm objecting to putting the new and the output file in different directories. But instead of a separate option, one could probably re-purpose "-o" for this.
I think "-a json" may fit more in the framework for now. Then you would get immediate feedback if it supported by the selected mode.
re a) i don't see json as a human readable format. And if you're processing it with any kind of tool, it should be possible to do timezone conversion there. I mean, decide on one format and use that. I don't really care if it's UTC or localtime as long as it is properly tagged.
OK!
re b) you can "cd" to the correct directory before running reassembler with the full path - which is what the "example.sh" already does. -- I am not objecting to putting the file in a different directory. I'm objecting to putting the new and the output file in different directories. But instead of a separate option, one could probably re-purpose "-o" for this.
Yeah, not wanting to cd into a dir for output, and certainly not wanting to store a script in the html content dir. Roger that on using -o
for it, and will make the move occur on same filesystem path.
I think "-a json" may fit more in the framework for now. Then you would get immediate feedback if it supported by the selected mode.
Reasonable enough.
Feedback has been addressed (and description above updated to reflect).
I haven't had time to test it, but I flagged two things I saw while scrolling through the changes. The unresolved %s in open looks incorrect, and I think it would have to be if 'json' in args: to not throw an error if "-a json" is not present. Additionally, I think you forgot to expand validargs= for the live-map case to accept json as option?
I haven't had time to test it, but I flagged two things I saw while scrolling through the changes. The unresolved %s in open looks incorrect, and I think it would have to be if 'json' in args: to not throw an error if "-a json" is not present. Additionally, I think you forgot to expand validargs= for the live-map case to accept json as option?
Thanks @Sec42 ... I've gone ahead and addressed both issues (and newly tested successfully).
Additionally, I think you forgot to expand validargs= for the live-map case to accept json as option?
Only adjustment for live-map's output was to change where existing goes, the json mode addition is only for acars.
While testing i realized that the live-map version now no longer worked if no "-o" was specified (due to ofile being initialized to /dev/stdout). I have pushed that part in a fixed version now as 91047043
I just realized while reviewing the rest, that I made a mistake in the ReassembleIDASBDACARS class by using self. - I'll have to change the code there. I'll try carry your json changes forward so you don't have to fix the conflicts yourself. Let you know in a bit. Sorry
Can you check out the new branch at https://github.com/muccc/iridium-toolkit/tree/kevinelliot-acarsjson and see if that behaves ok for you?
Can you check out the new branch at https://github.com/muccc/iridium-toolkit/tree/kevinelliot-acarsjson and see if that behaves ok for you?
Seems to be working for me.
Ok. If any changes are needed, maybe make a PR with that as the start? On the other hand if you can confirm that no changes are needed, I'll merge that branch as-is.
Ok. If any changes are needed, maybe make a PR with that as the start? On the other hand if you can confirm that no changes are needed, I'll merge that branch as-is.
I would say it's good to go.
Ok, I have merged that branch just now.
This adds JSON output to the
reassembler.py
for the mode ACARS (-m acars
). The intended purpose for this is so that people can shuttle this data around to other systems and services, such as (but not limited to) Airframes.io.Usage:
-j
or--json
enables the JSON output and disables the plain pretty text format.-a json
enables the JSON output and disables the plain pretty text format, when in ACARS mode (-m acars
)--station <STATION>
lets you set your station id (something likeKMHR4-IRIDIUM
or even a UUID), useful for aggregation services to id the source of the data--local-time
lets you toggle off UTC conversion for JSON output, (so all timestamps are in local time instead) -- useful for storing locally instead of sending up to an aggregator. For aggregation, UTC should be used.Also:
-o /some/path/output.json
so you can specify where thesats.json
is put when generated forlive-map
.Also changed how it moves the file so it supports cross-filesystem.Example command lines:
sudo iridium-extractor -D 4 --multi-frame ~/sat/airspy.conf 2>/dev/null | ~/iridium-toolkit/iridium-parser.py | python3 ~/iridium-toolkit/reassembler.py -i - -m acars -a json -o /srv/web/html/acars.json --station KMHR4-IRIDIUM
sudo iridium-extractor -D 4 --multi-frame ~/sat/airspy.conf 2>/dev/null | ~/iridium-toolkit/iridium-parser.py | python3 ~/iridium-toolkit/reassembler.py -i - -m live-map -o /srv/web/html/sats.json
Example ACARS json output:
Plan to follow up with future PRs for adding JSON output to other modes.