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.

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

Improve HotROD demo #915

Merged
merged 3 commits into from
Jul 8, 2018
Merged

Improve HotROD demo #915

merged 3 commits into from
Jul 8, 2018

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Jul 8, 2018

  • Allow controlling optimizations from cmd line
  • Suppress logging of stack traces unless fatal
  • Add hotrod namespace to all metrics
  • Support HTTP or UDP senders

On the topic of senders, the Go client, unfortunately, does not support JAEGER_ENDPOINT env variable like Java client does, and the current CLI argument is called --jaeger-agent.host-port, even though it now accepts HTTP endpoint name.

@ghost ghost assigned yurishkuro Jul 8, 2018
@ghost ghost added the review label Jul 8, 2018
@yurishkuro yurishkuro changed the title Detect HTTP payload format from Content-Type Improve HotROD demo Jul 8, 2018
@codecov
Copy link

codecov bot commented Jul 8, 2018

Codecov Report

Merging #915 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #915   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         126    126           
  Lines        6071   6071           
=====================================
  Hits         6071   6071

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89f3cca...7bfe37c. Read the comment docs.

Yuri Shkuro added 2 commits July 8, 2018 13:59
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
@@ -51,23 +58,38 @@ func Execute() {

func init() {
RootCmd.PersistentFlags().StringVarP(&metricsBackend, "metrics", "m", "expvar", "Metrics backend (expvar|prometheus)")
RootCmd.PersistentFlags().StringVarP(&jAgentHostPort, "jaeger-agent.host-port", "a", "0.0.0.0:6831", "String representing jaeger-agent host:port")
RootCmd.PersistentFlags().StringVarP(&jAgentHostPort, "jaeger-agent.host-port", "a", "0.0.0.0:6831", "String representing jaeger-agent UDP host:port")
Copy link
Contributor

Choose a reason for hiding this comment

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

should this also say that http host:port is accepted as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

we could say that, my main hesitation was that the property is called jaeger-agent.host-port, whereas an HTTP endpoint would be running in the collector, not the agent.

Copy link
Contributor

Choose a reason for hiding this comment

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

doh, in that case, should we just add a new flag for jaeger-client.reporting-host-port? and say the old one is deprecated? it is a demo app, not sure if many would care.

Copy link
Member Author

Choose a reason for hiding this comment

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

for now I added http to the description. I would rather not create another flag, as I think there's a better way - #920

Signed-off-by: Yuri Shkuro <ys@uber.com>
@yurishkuro yurishkuro merged commit e8cf3d1 into master Jul 8, 2018
@ghost ghost removed the review label Jul 8, 2018
@yurishkuro yurishkuro deleted the hotrod-demo branch July 8, 2018 22:13
RouteWorkerPoolSize = 3
// MySQLMutexDisabled controls whether there is a mutex guarding db query execution.
// When not disabled it simulates a misconfigured connection pool of size 1.
MySQLMutexDisabled = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Double negatives are confusing :-) How about MySQLMutexEnabled = true ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't double negative, but more importantly disabled plays nicer with false as the default value.

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