make formatters functions internal #126
Conversation
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.
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.
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.
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.
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 |
this PR proposes creating internal sub-packages for the 'formatter' functions that currently live in the
senderspackage. It turns out that each of the data types that the SDK can send has its own{metric,event,span,histogram}Linefunction 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
senderspackage 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.