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

Move log tokenizer to lib/strvals package #3446

Merged
merged 5 commits into from
Nov 10, 2023
Merged

Conversation

ka3de
Copy link
Contributor

@ka3de ka3de commented Nov 8, 2023

What?

Moves the tokenizer implementation, currently only used to parse the log output option configuration, to a more generic package.

Why?

Up until now the tokenizer implementation was only used to parse the log output option configuration. With our current intention of using this format for other options, such as traces output, it becomes evident that this implementation should be moved away from the log package and into a more generic one.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes. (N/A)
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Related #3445

Up until now the tokenizer implementation was only used to parse the log
output option configuration. With our current intention of using this
format for other options, such as traces output, it becomes evident that
this implementation should be moved away from the log package and into a
more generic one.
@ka3de ka3de marked this pull request as ready for review November 8, 2023 10:15
@github-actions github-actions bot requested review from mstoykov and oleiade November 8, 2023 10:15
mstoykov
mstoykov previously approved these changes Nov 8, 2023
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

My only nit is that I kind of don't like parseutils in part due to the utils part.

But tokenizer.Tokenize is no better as well as tokens.Parse or tokens.New

So unless @oleiade comes up wiht something better I guess this is it.

@mstoykov mstoykov added this to the v0.48.0 milestone Nov 8, 2023
@ka3de
Copy link
Contributor Author

ka3de commented Nov 8, 2023

My only nit is that I kind of don't like parseutils in part due to the utils part

Yep, to be honest I don't quite like it either. I considered parse or parseutil, but because there was already a testutils package, decided to be coherent with that. But open to any suggestion for another name.

@ka3de ka3de mentioned this pull request Nov 9, 2023
5 tasks
Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have explored a few options, most of which would turn out to require more effort, and I'd rather unblock you instead 🙇🏻

I would vote for making the name of the package as specific and explicit as possible, rather than broad and generic. Thus, I would suggest something along the lines of lib/logparser, or lib/kvparser package or something along those lines instead.

My reasoning is that the parsing mechanism described in the package is quite specific to the key=value,... format, which is meant to be used in logs, and I don't think that calling parseutils.Tokenize is very clear on what to expect from it: I'm gonna parse something, and tokenize it, but it feels generic when the implementation is actually specific to a given format.

I wouldn't block the PR over this though, and although I would find parseutils subpar, it would be good enough if we can't reach an agreement.

@mstoykov
Copy link
Contributor

mstoykov commented Nov 9, 2023

The original code was loosely based on what https://pkg.go.dev/k8s.io/helm/pkg/strvals does.

So I guess strvals is also kind of an option.

strvals.Parse seems to be by far the best option to my 👀.

but I am also okay with kvparser.Tokenize

I also prefer to not block @ka3de but if we have a clearly better name that we agree on it seems like a good idea to take 5 minutes. And yes parseutils is way too generic 👍

@oleiade
Copy link
Member

oleiade commented Nov 9, 2023

I like strvals 👍🏻 Let's go for that if you're okay with it @ka3de 🙇🏻

@ka3de
Copy link
Contributor Author

ka3de commented Nov 9, 2023

My reasoning is that the parsing mechanism described in the package is quite specific to the key=value,... format, which is meant to be used in logs, and I don't think that calling parseutils.Tokenize is very clear on what to expect from it

I guess this depends also on whether this package will hold more parsing implementations in the future. But as of now I agree that the implementation is pretty specific.

I would vote for making the name of the package as specific and explicit as possible, rather than broad and generic. Thus, I would suggest something along the lines of lib/logparser, or lib/kvparser package or something along those lines instead.

My understanding is that the format implemented by Tokenize is not strictly related with log/s. We are actually now, with this PR, using it to parse the traces output configuration. So to me kvparser is a better fit as of now 👍

Edit: On the strvals proposal.
If that works for you I have no objection. I will change it.

@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (e44a08f) 73.36% compared to head (f06e753) 73.33%.
Report is 1 commits behind head on master.

❗ Current head f06e753 differs from pull request most recent head b5a0ee0. Consider uploading reports for the commit b5a0ee0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3446      +/-   ##
==========================================
- Coverage   73.36%   73.33%   -0.03%     
==========================================
  Files         261      261              
  Lines       19742    19769      +27     
==========================================
+ Hits        14483    14498      +15     
- Misses       4365     4373       +8     
- Partials      894      898       +4     
Flag Coverage Δ
ubuntu 73.26% <79.16%> (-0.04%) ⬇️
windows 73.19% <79.16%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
cmd/root.go 94.23% <100.00%> (+0.23%) ⬆️
cmd/state/state.go 6.94% <100.00%> (+1.31%) ⬆️
lib/strvals/parser.go 100.00% <100.00%> (ø)
log/file.go 79.66% <100.00%> (ø)
log/loki.go 69.08% <100.00%> (ø)
cmd/run.go 74.38% <80.00%> (+0.11%) ⬆️
cmd/ui.go 69.65% <0.00%> (-0.71%) ⬇️
api/server.go 68.75% <40.00%> (-13.86%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mstoykov
mstoykov previously approved these changes Nov 10, 2023
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Just rename the PR title before merge ;)

@ka3de ka3de changed the title Move log tokenizer to lib/parseutils package Move log tokenizer to lib/strvals package Nov 10, 2023
@ka3de
Copy link
Contributor Author

ka3de commented Nov 10, 2023

I have no permission to merge the PR. Could you merge this please?

@mstoykov mstoykov merged commit 5e83d38 into master Nov 10, 2023
21 checks passed
@mstoykov mstoykov deleted the refactor/move-tokenizer branch November 10, 2023 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants