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

Bump datastation #85

Merged
merged 6 commits into from
Jul 24, 2022
Merged

Conversation

0michalsokolowski0
Copy link
Contributor

@0michalsokolowski0 0michalsokolowski0 commented Jul 18, 2022

  • Bump github.com/multiprocessio/datastation/runner (adding support for LogFmt logs)
  • Add test for LogFmt

@eatonphil
Copy link
Member

Looks good! Could you please add a test (and test file) see https://github.com/multiprocessio/dsq/pull/82/files for an example.

@0michalsokolowski0 0michalsokolowski0 changed the title Add logfmt Bump datastation Jul 19, 2022
@0michalsokolowski0
Copy link
Contributor Author

Added the test

@eatonphil
Copy link
Member

Yeah, this is failing on Windows for some reason. If you have a Windows machine feel free to debug there otherwise I'll get to it sometime :D

@0michalsokolowski0
Copy link
Contributor Author

I think I found two bugs here:

  1. From the error we can see that the command that is called looks like this:
    Command '['cat', '.\\testdata\\logfmt\\log.txt', '|', '.\\dsq.exe', '-s', 'text\\logfmt', 'SELECT level FROM {}']
    The most interesting part here is 'text\logfmt' found here when running on Windows we are replacing all / with \\ as a result text/logfmt becomes text\\logfmt

To avoid this we could replace:
if WIN and not doNotReplaceWin: for i, piece in enumerate(pieces): pieces[i] = piece.replace('./dsq', './dsq.exe').replace('/', '\\')
With:
if WIN and not doNotReplaceWin: for i, piece in enumerate(pieces): if piece.startswith('./dsq'): pieces[i] = piece.replace('./dsq', './dsq.exe') elif piece.startswith('./'): pieces[i] = piece.replace('/', '\\')

  1. After I changed the code locally another issue popped up: Unknown mimetype for file: <file_path>.
    The reason for this issue is that here we are calling resolveContentType which has a condition that looks strange on Windows.
    fileExtensionOrContentType is equal to text/logfmt and we are checking if it contains a separtor \\.
    Obviously it returns false and as a result UnknownMimeType is eventually returned.

I am not sure I understand the purpose of the condition strings.Contains(fileExtensionOrContentType, string(filepath.Separator)) I think what is should do is checking if the provided string is equal to a supported mime type and in that case return it.

If my interpretation is correct I would refactor MimeType in Datastation to have a function checking if a string can directly be mapped from string like: NewMimeTypeFromString(s string)

@eatonphil
Copy link
Member

@0michalsokolowski0 great work investigating this!

Let's avoid this for now by adding support for recognizing a .logfmt extension in DataStation here: https://github.com/multiprocessio/datastation/blob/main/runner/file.go#L584. I want to have this anyway. This way you can just ./dsq log.logfmt blah instead of cat logfmt.txt | ./dsq -s text/logfmt.

@0michalsokolowski0
Copy link
Contributor Author

@eatonphil eatonphil merged commit fa3183b into multiprocessio:main Jul 24, 2022
@eatonphil
Copy link
Member

Great work!

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

Successfully merging this pull request may close these issues.

None yet

2 participants