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

Namespace clashes because of generic naming #3

Closed
rhino232 opened this issue Mar 20, 2019 · 17 comments
Closed

Namespace clashes because of generic naming #3

rhino232 opened this issue Mar 20, 2019 · 17 comments

Comments

@rhino232
Copy link

We tried to use the falcon as our new http-server, but this failed because of a namespace clash.

After debugging it with pry we found out, that the built in logger module Event implemented in event.gem clashes with one of our core abstractions also called event.

Event is a very generic name for a nice improved logger gem.

We have two issues with the naming:

  • it does not convey the true meaning of the gem.
  • Additionally it runs a high risk to clash in the global namespace with other applications when it runs with falcon.

Is there any chance to rename the gem to another more specific name (e.g. event-logger), that implies more meaning and is more specific to the task it fullfills?

@ioquatix
Copy link
Member

ioquatix commented Mar 20, 2019

That’s actually a problem I never imagined having.

I guess you’ve got an AR model or something similar called Event. I always put my models under a module.

I spent like a week trying to find a short concise gem name for this project as most obvious names are all tied up on rubygems. Sometimes by frustratingly old and unmaintained projects.

You are right it’s super generic. But it does make sense. This is all about "events" in your application and logging them either using structured logging (e.g. JSON) or to the terminal in a human readable form. One of my longer term goals is to actually make a web interface to show these events as they happen in real time - so not a sequential set of strings, but more like a real time graph of events happening in your system.

I’m not really sure what the solution here is. Let me think about it. No matter what name I choose there is the potential to have this issue.

Calling the gem event-logger won't help because the module would be Event::Logger. It would have to be something completely different. I could remove the event-logger code and just falling back to some basic logger instance but the output would be much worse.

@ioquatix
Copy link
Member

ioquatix commented Mar 20, 2019

By the way, this is also pretty hilarious from the POV that this code started out in the following gems:

  • ????? lost to the sands of time
  • build
  • async/logger
  • tty-console (asked but didn't get permission to use tty- prefix).
  • fatal (broke JRuby).
  • advise
  • event
  • ?????

I finally found a name that I consider half decent event which was available as a gem (after I asked nicely) and reworked all my existing projects to use it, only to find out it's going to cause problems with people using class Event in the global namespace. Forgive me if I put my head in a box and scream for a few minutes, metaphorically :)

@ioquatix
Copy link
Member

Okay, after internal discussion, I think this is going to be the solution:

  • Add a per-task logger, defaults to parent task logger (late binding).
  • Add a per-reactor logger which defaults to Async.logger (late binding).
  • Make Async.logger default to an instance of stdlib Logger.
  • Make it easy to use a better logger (e.g. Event::Logger) but don't depend on it by default.

The only problem with this is that it will make logging really shitty by default, because stdlib logger is shitty by default, but maybe it's unavoidable. At least we can experiment with making it less bad.

@ioquatix
Copy link
Member

I'm also considering that Async.logger would be set up like this:

begin
  require 'event/console'
  @logger = Event::Console.logger
rescue LoadError
  require 'logger'
  @logger = Logger.new
end

But I'm not sure if that's a good approach or not.

@ioquatix ioquatix reopened this Mar 20, 2019
@rhino232
Copy link
Author

Yes we have an ActiveRecord Model called Event which wraps a legacy database.
We plan to rename it, because we have already internally more than one part that calls itself an event (comming from different products).
We will eventually rename it, but this is not happening anytime soon, and it takes substantially longer than two weeks.
We once had modules around this model, that triggered problems with autoloading. Eventually we clean it up, but unfortunately it is a major task.

With this explanation I would not mind to change our part and we will, but I am afraid other clients could run in to this same problem.

I personally would prefer the name async-logger, since it conveys the meaning of the gem and
hints at the context (async) in which it is used.

@ioquatix
Copy link
Member

ioquatix commented Mar 21, 2019

(Sorry for this wall of text but I wanted to summarise everything that I've done today for future reference)

It's already accepted that Async is complex enough that some kind of logger is required. That's indisputable. Even if it was just $stderr.puts but that's not very helpful as projects get more complex.

Here is what I tried today:

  • I submit PRs to Ruby stdlib logger to make it more compatible with how Async.logger works. Unfortunately we didn't make progress on the most important one which is buffered multi-line IO. Some others were merged which make the interface more compatible.

  • I've made a test build of Async which can use either stdlib Logger or Event::Console and it worked but for more complex use cases, it's broken. It's difficult to support two different (but similar) logging interfaces and I'm struggling to find a useful middle ground between stdlib Logger and Event::Logger, given that I've had the luxury of designing Event::Logger around a heap of real-world use cases.

I discussed this issue at length with @bryanp and I think we came to the conclusion that:

  • Async should be focused on what it needs to do which is concurrency and not logging. Having another gem dependency might be considered poor design, and I did anguish over that a little bit when I first split out Async::Logger.

  • That being said, I still believe good logging is really important. If I decided to choose to use stdlib Logger by default, the user experience out of the box is impacted because the error reporting and logging experience is so much worse for developers and it's very hard to improve on it without a parallel implementation of Event sitting in Async::Logger. I think we can all agree clear and useful error messages are super critical in general code, and even more so in code which is executing concurrently (i.e. don't silently ignore/consume errors).

  • If stdlib Logger was the default, it could be possible to use LoadError to try loading Event::Console first and then falling back to Logger. But this would require action on the part of the user to get nicer logging and I think it would mean it wouldn't be used as much and I personally want really good logging in my own projects by default without having to add other gems. I want developers to use Async and be really blown away by the great logging and error reporting right out of the box.

  • Async should have a clean interface for user supplied logging (would love to get feature parity between Event::Logger and stdlib Logger but it's not looking great).

  • It should be possible to specify per-request (i.e. per-task) loggers.

  • The formatting of Async.logger was a bit surprising out of the box (maybe not in a good way?). I think this is something we should focus our energy on - making this as awesome as possible since it's often one of the first things that mitigates the frustrating bugs that can be caused by asynchronous code/program flow.

I think ultimately while I agree with you in many ways, I have too many conflicting interests. My one overriding interest is that Async should have the best possible of out of the box experience for developers.

The reason for this is when I used celluloid, logging was painful. Sometimes actors would die and you didn't know why, or the state would be hard to debug. Logging is critical as systems get more complex and less "synchronous".

The Rails/AR approach of dumping everything into the global namespace is bound to cause pain and other issues and I'm not sure I can justify a huge complexity w.r.t logging just to work around that. I already spent a lot of time on it today which was taken away from other more important issues. It sounds like the right solution here is to rename it on your end, or even better put it in a module as you suggested.

On the other hand, I could just rename the gem to something obscure, but that's not ideal either. Name clash is inevitable due to the design of Ruby. Unfortunately event is both great because it's terse and straight forward, but terrible because it's so generic and could clash with user code. I think most of Socketry code is under Async module except the logger, which in this case makes it stick out like a sore thumb.

At this point, I see benefits to this discussion, but I'm not sure if the cost of supporting stdlib Logger, especially given the "why should we do this" response to my PRs today. We ideally need the interface to have feature parity with Event::Logger (really just very minor, non-breaking, but super convenient changes). If I got a better response from stdlib Logger, I would be more enthusiastic about it, but as it stands it's about as enjoyable as eating raw onions.

I wish I could find some better solution. I wish stdlib Logger was much better, then it would be a no-brainer to use it. But as it stands, it's not that great as far as generic logging interfaces go. We need a Rack SPEC for loggers - that allows us to have lots of different implementations that adhere to the same interface.

@ioquatix
Copy link
Member

Pinging @wmorgan who seems to be the owner of the console gem with the last release on Rubygems in 2011 - would you be willing to give up that namespace and help resolve some of the above issues?

@wmorgan
Copy link

wmorgan commented Mar 25, 2019

Can you walk me through what you're asking me to do? I am very far removed from the Ruby world these days.

@ioquatix
Copy link
Member

ioquatix commented Mar 25, 2019

@wmorgan you have ownership of a gem named console which would be great name for this gem and help resolve this issue.

If you are no longer using that name, you can transfer it to me, which would be so awesome!

gem owner console -a samuel@codeotaku.com

Let me know if you have any further questions.

@ioquatix
Copy link
Member

@wmorgan any chance of an update? Thanks :)

@wmorgan
Copy link

wmorgan commented Mar 29, 2019

Would transferring this to you mean that require 'console' statements in existing code would suddenly do something very different?

@ioquatix
Copy link
Member

I am happy to maintain the existing release of the code if you provide it to me.

There is only one reverse dependency listed: https://rubygems.org/gems/doctor

It has a hard dependency on the latest release (v0.5), so it won't break because I'll leave the existing releases alone. Therefore, for that dependency, it won't break anything.

If there are private projects that depend on it, I am happy to maintain the current interface, but within the bounds of semantic versioning (so a major version bump might change the interface, for example). But this should already be compatible with how projects use this gem.

@wmorgan
Copy link

wmorgan commented Mar 29, 2019

Ok, ownership transferred!

@ioquatix
Copy link
Member

@wmorgan are you able to provide me the original source code?

@wmorgan
Copy link

wmorgan commented Mar 29, 2019

Hm, that will require a little digging. Give me a few days.

@ioquatix
Copy link
Member

@wmorgan Thanks, just keep me in the loop!

@rhino232 I believe we will be able to address the name clash you mentioned, so I'm going to close this issue.

@rhino232
Copy link
Author

rhino232 commented Mar 29, 2019 via email

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

No branches or pull requests

3 participants