-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add HTTP helper for Metricsets #3413
Conversation
client *http.Client // HTTP client that is reused across requests. | ||
} | ||
|
||
// NewHttpMetricset creates new instance of HttpMetriSset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type in HttpMetricSet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks more like the comment is out of date, it should be "creates a new instance of Http". Btw, HTTP is an acronym so usually it shows up as HTTP in Go code, right? That might look weird though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, in the first version it was call HttpMetricset because it was not in the metricset package.
You are right based on our standard it should be called HTTP
, but it will look wiered :-( I would prefer to leave it. @urso Thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, this is great. It now makes sense to add some unit tests for the Http type. I'm good with having it in a future PR.
6daf205
to
0ca6c80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice improvement!
"github.com/elastic/beats/metricbeat/mb" | ||
) | ||
|
||
type Http struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Http
should be HTTP
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:-) @tsg and me had the same discussion, but came to the conclusion HTTP will look ugly :-) What would you call the private variable in this case? http?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What private variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see anything wrong with http *helper.HTTP
. It's not much different from http *helper.Http
. Or am I missing something?
This should simplify the implementation of MetricSets based on HTTP.
This should simplify the implementation of MetricSets based on HTTP.