-
Notifications
You must be signed in to change notification settings - Fork 7
Conversation
I did look at updating code to the otel version but then realised that |
@paulosman @lizthegrey couldn't seem to assign reviewers, would you mind taking a look. Have tested my fork against Honeycomb and all seems to be injesting fine from a client perspective. |
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.
One small non-blocking comment. LGTM! Thanks so much for tackling this.
example/client/client.go
Outdated
@@ -38,52 +40,56 @@ func main() { | |||
dataset := flag.String("dataset", "opentelemetry", "Your Honeycomb dataset") | |||
flag.Parse() | |||
|
|||
exporter, err := honeycomb.NewExporter( | |||
exporter, errExp := honeycomb.NewExporter( |
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.
Is there a reason to change this? If not, I'd prefer to leave it as it was, naming this err
and using err = func(ctx context.Context) error {}
below.
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.
Ah, will fix that ... Just habit so err doesn't get shadowed.
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.
revert it back ... it is only example code :)
@paulosman im not sure if any other updates are needed for it to be 0.11.0 compatible and tagged, I did update the version in preperation. Actually need to update the |
Change
kv
tolabel
as this has been updated in OTel0.11.0
.Release Notes https://github.com/open-telemetry/opentelemetry-go/releases/tag/v0.11.0
See open-telemetry/opentelemetry-go#1060
Side Note:
codes should also be updated but would probably better do it in a different PR.