Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Metricbeat] Rename Logger method to Log #11126

Closed
wants to merge 1 commit into from

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Mar 7, 2019

Based on the comment #11106 (comment) the Logger method is renamed to Log. It turned out this was already almost a convention which we used across the metricsets. This meant the Log variable and Log() method conflicted and required an update in the metricsets that had a Log variable.

No functionality was changed in this PR.

Based on the comment elastic#11106 (comment) the Logger method is renamed to Log. It turned out this was already almost a convention which we used across the metricsets. This meant the Log variable and Log() method conflicted and required an update in the metricsets that had a Log variable.

No functionality was changed in this PR.
@ruflin ruflin added the in progress Pull request is currently in progress. label Mar 7, 2019
@ruflin
Copy link
Contributor Author

ruflin commented Mar 7, 2019

Put to in progress again as we should discuss again if Log or Logger is better here.

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM 😄

@@ -275,7 +275,7 @@ func (b *BaseMetricSet) Metrics() *monitoring.Registry {
}

// Logger returns the logger.
Copy link
Member

Choose a reason for hiding this comment

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

The godoc here will need updated too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like you are better then hound :-)

@urso
Copy link

urso commented Mar 7, 2019

+1 on shorter names

@fearful-symmetry
Copy link
Contributor

+1, Seems good.

@cachedout
Copy link
Contributor

+1 but this does raise the question of whether it would be good to document these sorts of conventions in the developer documentation.

@ruflin
Copy link
Contributor Author

ruflin commented Mar 8, 2019

@cachedout Yes, @fearful-symmetry has some ideas plans around this.

@sayden
Copy link
Contributor

sayden commented Apr 4, 2019

My 2 cents: This breaks common naming conventions and I don't think that by documenting it, it will help to understand code better and faster, let me explain:

  • Log() object [verb/action] method or function is expected to return a log line of text (object), based on some state.
  • Log(arg) [verb/action] is expected to log arg at the default level (like t.Log in testing package). If you add Log(arg) object is usually expected that object is going to be an object or an error.
  • Log() [verb/action] is expected to log something at the default level based on some state: accumulated logs, for example, based on different log strategies, not just to print to stdout but it might be to Logstash and the destination and complexity is hidden / abstracted from the caller.
  • Log [object] variable is expected to contain a piece of text, a log.
  • Logger interface is expected to contain the methods needed to implement a logger.
  • Logger() object [noun] method is expected to return an interface with an initialized logger which implements Logger interface, based on some state or not. Function call is expected to initialize and return a new Logger which also implements the same interface.
  • Logger(arg) object [noun] method or function is expected to return a Logger initialized with something based on arg.
  • Logger [noun] variable, is expected to container a implementor of Logger interface.
  • NewLogger() [action-noun] is expected to initialize a new Logger. NewLogger() object is expected to initialize a new Logger and return it, but not store it.

As you can see, actions/verbs are usually function or type methods while nouns are usually types. This methodology is actually quite old (I think from the times of UML) but it's still valid today and makes a lot of sense.

+1 on shorter names if they don't affect code comprehension for new arrivals or community contributors. All in all that's why we all have autocomplete in our IDE 😉

@ruflin
Copy link
Contributor Author

ruflin commented Apr 29, 2019

Closing this PR as it will not make a big difference from my point of view. If someone wants to take over, please go for it :-)

@ruflin ruflin closed this Apr 29, 2019
@zube zube bot removed the [zube]: Ready label Apr 29, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
in progress Pull request is currently in progress. Metricbeat Metricbeat module review Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants