Skip to content

Conversation

sagikazarmark
Copy link
Contributor

This is an experimental PR suggested by @basvanbeek in #946 (comment)

It adds a span name getter function to the opencensus endpoint tracer config.

Other options might worth adding:

  • StartOptions
  • GetStartOptions

See the opencensus ochttp plugin for more.

@sagikazarmark
Copy link
Contributor Author

What are your thoughts about this? Is this what you had in mind?

@peterbourgon
Copy link
Member

@basvanbeek

@sagikazarmark
Copy link
Contributor Author

@basvanbeek I've updated the PR based on your comments.

However, I feel it would be better to split the function into two:

  • GetSpanName
  • GetAttributes

More functions, more work, but they are more expressive IMHO.

@basvanbeek
Copy link
Member

basvanbeek commented Jan 31, 2020

We can do the split... we can also say that if a returned name is empty on a call to GetName we keep the existing name for the endpoint. so you can drop the in parameter for name.

@sagikazarmark
Copy link
Contributor Author

Updated the PR accordingly. If you have any ideas for better naming, let me know.

Copy link
Member

@basvanbeek basvanbeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic looks good to me with one nit as shown above. Also unsure of method naming. @peterbourgon any ideas?

@peterbourgon peterbourgon merged commit cc938d5 into go-kit:master Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants