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

Store messages as swift-log backend #2

Merged
merged 6 commits into from May 6, 2020
Merged

Conversation

slashmo
Copy link
Contributor

@slashmo slashmo commented May 5, 2020

This is still very much WIP, but should be enough to start a deeper discussion around whether this makes sense for Pulse. I've inlined some #warning()s for points that need to be considered.

Questions

@slashmo
Copy link
Contributor Author

slashmo commented May 5, 2020

@kean I removed the Linux test files from the Swift package as this project is for Apple platforms only anyways and those files would only have to be updated each time the tests are updated.

@kean
Copy link
Owner

kean commented May 5, 2020

  1. When/where should sweeping be scheduled?

I think it should be moved to LoggerMessageStore init. expirationInterval should be a parameter of LoggerMessageStore too.

  1. Can system be replaced by swift-logs labels?

Yes, it should be replaced by label, and category should be removed - you can use metadata instead. Feel free to make breaking changes in the data model, nobody is using this yet.

  1. Should we store swift-logs metadata as well?

Yes, see my comment.

  1. What's the equivalent of category in PersistentLogHandler?

See answer to 2.

@kean
Copy link
Owner

kean commented May 5, 2020

Thanks for putting this together, @slashmo. This is a fantastic change, I agree with all of the changes that you made, including renaming the components.

Thanks for adding some unit tests too. I enabled Travis CI for this repo. All it needs a configuration file now. Feel free to add it if you are interested, or I could do that later today once this is merged.

@kean kean mentioned this pull request May 6, 2020
@slashmo
Copy link
Contributor Author

slashmo commented May 6, 2020

I enabled Travis CI for this repo. All it needs a configuration file now. Feel free to add it if you are interested, or I could do that later today once this is merged.

I guess this can be tracked in a separate issue. I haven't worked with Travis recently, though I could help with setting up GitHub Actions if you'd want to use that instead.

@kean
Copy link
Owner

kean commented May 6, 2020

A good idea, let me try using GitHub Actions for this repo. I'll track it separately.

@slashmo
Copy link
Contributor Author

slashmo commented May 6, 2020

@kean I think this can transition into a real PR now. I guess the only thing left to update is the setup instructions in the README.

@kean
Copy link
Owner

kean commented May 6, 2020 via email

@slashmo slashmo marked this pull request as ready for review May 6, 2020 18:23
@kean kean merged commit 956937f into kean:master May 6, 2020
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.

Using Apples swift-log
2 participants