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

feat: add -T/--timestamps (env LOGPROXY_TIMESTAMPS) option to prepend strftime to each line #35

Merged
merged 2 commits into from
Feb 8, 2024
Merged

Conversation

mk-fg
Copy link
Contributor

@mk-fg mk-fg commented Feb 5, 2024

Might work to address [ ] option to add a timestamp before each log line in the README.
Not sure if it's the best way to do it, but was easy enough to add this way, to address my own use-case.
Feel free to merge with any kind of edits and tweaks, preserving commit/attribution does not matter to me at all.

Thanks for making this small tool.
It's especially useful in modern non-shell invocations with a proper wrapper mode and configuration via env vars.

@CLAassistant
Copy link

CLAassistant commented Feb 5, 2024

CLA assistant check
All committers have signed the CLA.

@thebaptiste
Copy link
Contributor

thebaptiste commented Feb 8, 2024

Many thanks you for your PR.
It works perfectly when the new option -T is used. But without this option, every log line starts with (null) !
It's because, in spawn_logproxy_async, with -T \"%s\", the NULL value for timestamp_prefix is replaced by (null) and passed to log_proxy
Adding this test in log_proxy main fix the problem :
if ( strcmp( timestamp_prefix, "(null)" ) == 0 ) timestamp_prefix = NULL;
I didn't find any better way to fix it, so I will take your PR with this fix.

@thebaptiste thebaptiste merged commit 262d47a into metwork-framework:master Feb 8, 2024
2 checks passed
@mk-fg
Copy link
Contributor Author

mk-fg commented Feb 8, 2024

Thanks.

My bad wrt prefix arg, added an option to wrapper as an afterthought and didn't think to test it without prefix.
Other fix can probably be using something like prefix_args = timestamp_prefix ? sprintf("-T \"%s\"", timestamp_prefix) : "" in the wrapper, and templating that into the command line.
And I think it'd be easier if wrapper wasn't using a string command, but an array of arguments, where such stuff can be appended or not as-needed, as also suggested in #37 for a different reason.

@thebaptiste
Copy link
Contributor

I'm not the lead maintener of log_proxy (@thefab is very busy these days) and you're probably a better python developer than me !
So feel free to open any improvement PR.

Meantime I will release 0.6.0 with your new option (which by the way was in our roadmap since several years !)

Thanks again

@mk-fg
Copy link
Contributor Author

mk-fg commented Feb 8, 2024

It's C, not python, but yeah sure, I'll make a PR today or tomorrow.

@thebaptiste
Copy link
Contributor

thebaptiste commented Feb 8, 2024

Yes C ! So I will wait for your PR before releasing.

@mk-fg
Copy link
Contributor Author

mk-fg commented Feb 8, 2024

Created it as #38, which should address this issue and #37, as described there.

mergify bot pushed a commit that referenced this pull request Feb 9, 2024
…ing locks (#40)

Earlier implementation in #35 can loop while holding the lock, which is not a good idea, and composing prefix+line into one GString is simpler anyway.
Using glib datetime and string formatting also allows to simplify implementation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants