-
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
make formatters functions internal #126
make formatters functions internal #126
Conversation
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.
Great work LukeWinikates. I like the way you were able to use shorter names because you moved them into packages under internal. I just have one concern.
senders/types.go
Outdated
@@ -58,3 +41,7 @@ type EventSender interface { | |||
// Sends an event to Wavefront with optional tags | |||
SendEvent(name string, startMillis, endMillis int64, source string, tags map[string]string, setters ...event.Option) error | |||
} | |||
|
|||
type SpanTag = span.Tag |
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.
SpanTag and SpanLog are types used in public interfaces of the senders package. Does it make sense to declare them as equivalent to internal types? I am not yet sure how godoc will handle this, but if I were learning this API, I'd want to see the full definition of SpanTag and SpanLog just because they are part of public interfaces.
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, that's the only thing that was 'hard' about this set of changes, because having the internal package depend on the senders package created a circular dependency. I agree we do want the full definition in godoc. I'll try rendering the godoc locally and see whether we get the full type definition there.
There's definitely some kind of smell happening. I want the senders
package to own all the public apis, and most of the line handlers only depend on primitive types. Spans are the only ones where the line formatter wants to use a custom struct type.
The struts are very simple so I suppose a translation from SpanTag
to span.Tag
would be easy to write, with both structs having the same fields.
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, you should be able to move between SpanTag and span.Tag using a simple type conversion.
I took a look at godoc, and confirmed that with the |
senders/client.go
Outdated
@@ -164,21 +164,15 @@ func (sender *wavefrontSender) SendSpan(name string, startMillis, durationMillis | |||
func makeSpanTags(tags []SpanTag) []span.Tag { | |||
var spanTags []span.Tag |
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.
Since you know what the length of your returned slice will be ahead of time, you could just say spanTags := make([]span.Tag, 0, len(tags))
here. Doing that will save Go from having to resize your slice as you add elements to it. The initial length of your spanTags slice will be 0, but the capacity of it will be len(tags) so no resizing will happen as you add elements with append.
senders/client.go
Outdated
Key: tag.Key, | ||
Value: tag.Value, | ||
}) | ||
spanTags = append(spanTags, span.Tag(tag)) | ||
} | ||
return spanTags | ||
} | ||
|
||
func makeSpanLogs(logs []SpanLog) []span.Log { | ||
var spanLogs []span.Log |
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.
Same here.
this PR proposes creating internal sub-packages for the 'formatter' functions that currently live in the
senders
package. It turns out that each of the data types that the SDK can send has its own{metric,event,span,histogram}Line
function and supporting functions. Splitting them apart so that they are in smaller, isolated files seems to make it easier to tell what tests go with what and which functions are used where.this also allows the
senders
package to focus on creating senders by moving serialization concerns out of that package.another observation: this also looks like rather than
foo.Line(foo.field1, foo.field2, foo.fieldN...)
each sendable type could have aLine()
method.