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

Add ability to adjust static sampling probabilities per operation #827

Merged
merged 3 commits into from
May 18, 2018

Conversation

black-adder
Copy link
Contributor

@black-adder black-adder commented May 17, 2018

Signed-off-by: Won Jun Jang [email protected]

Which problem is this PR solving?

Short description of the changes

@@ -81,7 +81,7 @@ def main():
for f in go_files:
parsed, imports_reordered = parse_go_file(f)
if output == "stdout" and imports_reordered:
print f + " imports out of order"
print(f + " imports out of order")
Copy link
Contributor Author

@black-adder black-adder May 17, 2018

Choose a reason for hiding this comment

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

i somehow got python 3 installed on my machine and this is backwards compatible with python 2 anyway

@coveralls
Copy link

coveralls commented May 17, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 2489884 on per_operation_static_sampling into cf242c3 on master.

@@ -21,9 +21,16 @@ type strategy struct {
Param float64 `json:"param"`
}

// operationStrategy defines a operation specific sampling strategy.
Copy link
Member

Choose a reason for hiding this comment

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

an operation

@yurishkuro
Copy link
Member

is there an issue that should be linked to this PR?

Signed-off-by: Won Jun Jang <[email protected]>
@objectiser
Copy link
Contributor

Not necessarily in scope for this PR, but I think we will also eventually need to take into account tags - for example, depending upon the instrumentation used, we may get a span reported with operation=GET and a tag http.url=http://xyz/health - the operation is too generic to isolate the specific endpoint that we want to specify the sampling config for e.g. /health.

Reason for raising here is more to do with how the strategy is modeled - it now includes "operation_strategies" - possibly want to make this more generic so could in the future be based on operation and/or a set of tags?

@yurishkuro
Copy link
Member

@objectiser we've always shied away from trying to infer operation name from the URL, due to REST practice of encoding IDs into the path. The recommendation is to fix the instrumentation to use route as the span name, instead of the http method.

@ghost ghost assigned black-adder May 17, 2018
@ghost ghost added the review label May 17, 2018
@black-adder black-adder merged commit 9d05fad into master May 18, 2018
@black-adder black-adder deleted the per_operation_static_sampling branch May 18, 2018 03:36
@ghost ghost removed the review label May 18, 2018
mabn pushed a commit to mabn/jaeger that referenced this pull request May 28, 2018
* master: (38 commits)
  Preparing release 1.5.0 (jaegertracing#847)
  Add bounds to memory storage (jaegertracing#845)
  Add metric for debug traces (jaegertracing#796)
  Change metrics naming scheme (jaegertracing#776)
  Bump gocql version (jaegertracing#829)
  Remove ParentSpanID from domain model (jaegertracing#831)
  Make gas run quiet (jaegertracing#838)
  Revert "Make gas run quite"
  Revert "Install gas from install-ci"
  Install gas from install-ci
  Make gas run quite
  Add 'gas' for security problems scanning (jaegertracing#830)
  Add ability to adjust static sampling probabilities per operation (jaegertracing#827)
  Support log-level flag on agent (jaegertracing#828)
  Remove unused function (jaegertracing#822)
  Add healthcheck to standalone (jaegertracing#784)
  Do not use KeyValue fields directly and use KeyValues as decorator only (jaegertracing#810)
  Add ContaAzul to the adopters list (jaegertracing#806)
  Add ISSUE_TEMPLATE and PULL_REQUEST_TEMPLATE (jaegertracing#805)
  Upgrade to  go 1.10 (jaegertracing#792)
  ...

# Conflicts:
#	cmd/agent/app/builder.go
#	cmd/collector/main.go
#	cmd/query/main.go
#	cmd/standalone/main.go
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.

Can we ignore spans for specific operations?
4 participants