-
Notifications
You must be signed in to change notification settings - Fork 21
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 event support #4
Conversation
This avoids the need for a String() function.
Clients can still use the simpler Info, Error, etc. abstractions for common cases. If more metadata is required on events, clients can use the more flexible (yet more verbose) Event.
Thank you @mkobetic for the original contribution! @mkobetic @sidgopalan would appreciate your feedback. I just noticed #3 as well. Looks like you have your choice of pulls for this feature 😛 |
AlertType: alertType, | ||
Tags: tags, | ||
} | ||
// Use the given client namespace as the source type name, if given |
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.
Hm, why did you decide to move this here. Given that this function is private, any client wishing to use the extended Event API, will miss this additional processing. Is that intentional?
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.
The intent is to give clients that use Event full control, but I see what you mean about missing out on the default namespace-based SourceTypeName if it's left empty on the struct. My first inclination is to modify Event(...) to use EventOpts.SourceTypeName if it's not empty and otherwise fallback to the Client.Namespace behavior. A possible problem is that this leaves no way for the client to omit SourceTypeName from the message entirely. Is that something we care about?
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.
That sounds reasonable, I think what you want to avoid more is not having any source info, whether from the client or from the API call.
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.
OK, makes sense. I'll plan to get another pull in that does that within the next week or so.
Otherwise LGTM. Thanks! |
} | ||
func (c *Client) Event(title string, text string, eo *EventOpts) error { | ||
var b bytes.Buffer | ||
fmt.Fprintf(&b, "_e{%d,%d}:%s|%s|t:%s", len(title), len(text), title, text, eo.AlertType) |
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.
Do we know if datadog wants the byte length of title
and text
or the character length? If the latter this should be len([]rune(title))
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.
Good catch! The documentation isn't specific about this, but the official Ruby implementation strongly suggests this should be character length. The code uses string#length, which returns character rather than byte length. I'll open another pull with a fix.
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.
In that case I'd use http://golang.org/pkg/unicode/utf8/#RuneCountInString, rather than copying the string into a []rune.
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.
Agreed: #5
Thanks @sidgopalan! I apologize for dropping the ball on the review response, and I'm grateful to @andrei-m for stepping in. |
Implements code review feedback from #2