Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Handle zipkin collector creation error #2849

Closed
wants to merge 1 commit into from
Closed

Handle zipkin collector creation error #2849

wants to merge 1 commit into from

Conversation

ferhatelmas
Copy link
Contributor

@ferhatelmas ferhatelmas commented Feb 12, 2018 •
edited by ldez

What does this PR do?

Handle skipped error.

Motivation

According to current implementation, there is no need to check for error.
However, it's future proof to handle error since signature signals that err is possible.

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

None.

@ferhatelmas
Handle zipkin collector creation
4d602a7 
 - according to current implementation, there is
no need to check for error. However, it's future proof
to handle error since signature signals that err is possible.
@ferhatelmas ferhatelmas changed the title Handle zipkin collector creation Handle zipkin collector creation error Feb 12, 2018
@ldez
Copy link
Member

ldez commented Feb 12, 2018

Thanks for the contribution 👍

We must declined this change, because this can be problem in the future.

@ldez ldez closed this Feb 12, 2018
@ferhatelmas
Copy link
Contributor Author

ferhatelmas commented Feb 12, 2018 •
edited

@ldez Sure. For my learning, I would appreciate if you can explain how it could be a problem. Thanks!

@ferhatelmas ferhatelmas deleted the zipkin-collector-err branch February 12, 2018 22:33
@ldez
Copy link
Member

ldez commented Feb 12, 2018 •
edited

In this case we must see zipkin.NewHTTPCollector(c.HTTPEndpoint) as a black box:

  • this method define a contract by this return
  • the behavior of the method can change in the future release of zipkin and return a real error instead of nil

@ldez
Copy link
Member

ldez commented Feb 13, 2018 •
edited

@ferhatelmas Sorry, I misunderstood... could you reopen this one ?

@ferhatelmas ferhatelmas restored the zipkin-collector-err branch February 14, 2018 20:29
@ferhatelmas ferhatelmas mentioned this pull request Feb 14, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
area/middleware size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ferhatelmas @ldez @traefiker