Skip to content

api: update the tracer api #131

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

Merged
dmehala merged 1 commit into from
Jul 8, 2024
Merged

api: update the tracer api #131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions examples/http-server/proxy/proxy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,36 +51,36 @@ int main() {
httplib::Response& res) {
tracingutil::HeaderReader reader(req.headers);
auto span = tracer.extract_or_create_span(reader);
span->set_name("forward.request");
span->set_resource_name(req.method + " " + req.path);
span->set_tag("network.origin.ip", req.remote_addr);
span->set_tag("network.origin.port", std::to_string(req.remote_port));
span->set_tag("http.url_details.path", req.target);
span->set_tag("http.route", req.path);
span->set_tag("http.method", req.method);
span.set_name("forward.request");
span.set_resource_name(req.method + " " + req.path);
span.set_tag("network.origin.ip", req.remote_addr);
span.set_tag("network.origin.port", std::to_string(req.remote_port));
span.set_tag("http.url_details.path", req.target);
span.set_tag("http.route", req.path);
span.set_tag("http.method", req.method);

httplib::Error er;
httplib::Request forward_request(req);
forward_request.path = req.target;

tracingutil::HeaderWriter writer(forward_request.headers);
span->inject(writer);
span.inject(writer);

upstream_client.send(forward_request, res, er);
if (er != httplib::Error::Success) {
res.status = 500;
span->set_error_message(httplib::to_string(er));
span.set_error_message(httplib::to_string(er));
std::cerr << "Error occurred while proxying request: " << req.target
<< "\n";
} else {
tracingutil::HeaderReader reader(res.headers);
auto status = span->read_sampling_delegation_response(reader);
auto status = span.read_sampling_delegation_response(reader);
if (auto error = status.if_error()) {
std::cerr << error << "\n";
}
}

span->set_tag("http.status_code", std::to_string(res.status));
span.set_tag("http.status_code", std::to_string(res.status));
};

httplib::Server server;
Expand Down
12 changes: 4 additions & 8 deletions examples/http-server/server/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,13 +207,9 @@ void on_request_headers_consumed(const httplib::Request& request, dd::Tracer& tr
config.start = context->request_start;

tracingutil::HeaderReader reader{request.headers};
auto maybe_span = tracer.extract_or_create_span(reader, config);
if (dd::Error* error = maybe_span.if_error()) {
std::cerr << "While extracting trace context from request: " << *error << '\n';
// Create a trace from scratch.
context->spans.push(tracer.create_span(config));
} else {
context->spans.push(std::move(*maybe_span));
{
auto span = tracer.extract_or_create_span(reader, config);
context->spans.push(std::move(span));
}

dd::Span& span = context->spans.top();
Expand All @@ -237,7 +233,7 @@ void on_healthcheck(const httplib::Request& request, httplib::Response& response
// We'd prefer not to send healthcheck traces to Datadog. They're
// noisy. So, override the sampling decision to "definitely
// drop," and don't even bother creating a span here.
context->spans.top().trace_segment().override_sampling_priority(int(dd::SamplingPriority::USER_DROP));
context->spans.top().trace_segment().override_sampling_priority(dd::SamplingPriority::USER_DROP);

response.set_content("I'm still here!\n", "text/plain");
}
Expand Down
4 changes: 4 additions & 0 deletions src/datadog/trace_segment.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,10 @@ void TraceSegment::span_finished() {
tracer_telemetry_->metrics().tracer.trace_segments_closed.inc();
}

void TraceSegment::override_sampling_priority(SamplingPriority priority) {
override_sampling_priority(static_cast<int>(priority));
}

void TraceSegment::override_sampling_priority(int priority) {
SamplingDecision decision;
decision.priority = priority;
Expand Down
2 changes: 2 additions & 0 deletions src/datadog/trace_segment.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#include "optional.h"
#include "propagation_style.h"
#include "sampling_decision.h"
#include "sampling_priority.h"
#include "tracer_telemetry.h"

namespace datadog {
Expand Down Expand Up @@ -147,6 +148,7 @@ class TraceSegment {
// Set the sampling decision to be a local, manual decision with the specified
// sampling `priority`. Overwrite any previous sampling decision.
void override_sampling_priority(int priority);
void override_sampling_priority(SamplingPriority priority);

private:
// If `sampling_decision_` is null, use `trace_sampler_` to make a
Expand Down
12 changes: 6 additions & 6 deletions src/datadog/tracer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,17 +310,17 @@ Expected<Span> Tracer::extract_span(const DictReader& reader,
return span;
}

Expected<Span> Tracer::extract_or_create_span(const DictReader& reader) {
Span Tracer::extract_or_create_span(const DictReader& reader) {
return extract_or_create_span(reader, SpanConfig{});
}

Expected<Span> Tracer::extract_or_create_span(const DictReader& reader,
const SpanConfig& config) {
Span Tracer::extract_or_create_span(const DictReader& reader,
const SpanConfig& config) {
auto maybe_span = extract_span(reader, config);
if (!maybe_span && maybe_span.error().code == Error::NO_SPAN_TO_EXTRACT) {
return create_span(config);
if (maybe_span) {
return std::move(*maybe_span);
}
return maybe_span;
return create_span(config);
}

} // namespace tracing
Expand Down
6 changes: 3 additions & 3 deletions src/datadog/tracer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,9 @@ class Tracer {
// new trace (see `create_span`). Optionally specify a `config` indicating
// the attributes of the span. If a failure occurs, then return an error.
// Note that the absence of a span to extract is not considered an error.
Comment on lines 76 to 78
Copy link
Copy Markdown
Contributor

@dgoffredo dgoffredo Jul 9, 2024

Choose a reason for hiding this comment

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

  // Return a span extracted from the specified `reader` (see `extract_span`).
  // If there is no span to extract, or if an error occurs during extraction,
  // then return a span that is the root of a new trace (see `create_span`).
  // Optionally specify a `config` indicating the attributes of the span.

Fight the entropy!

At least, that's what the name of the function suggest and I think the desired behavior

Desired by whom? The advantage of allowing extraction errors to be returned is that the caller can, if desired, log the error. I anticipated "traces/sampling broken in Istio thingy" bugs whose root cause is extraction failing without making any noise. Likely you weighed this possibility and decided the benefit of Expected<Span> → Span outweighed the risk.

Expected<Span> extract_or_create_span(const DictReader& reader);
Expected<Span> extract_or_create_span(const DictReader& reader,
const SpanConfig& config);
Span extract_or_create_span(const DictReader& reader);
Span extract_or_create_span(const DictReader& reader,
const SpanConfig& config);

// Return a JSON object describing this Tracer's configuration. It is the same
// JSON object that was logged when this Tracer was created.
Expand Down
13 changes: 4 additions & 9 deletions test/system-tests/request_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,16 +102,11 @@ void RequestHandler::on_span_start(const httplib::Request& req,
if (auto http_headers = utils::get_if_exists<nlohmann::json::array_t>(
request_json, "http_headers")) {
if (!http_headers->empty()) {
auto maybe_span = tracer_.extract_or_create_span(
auto span = tracer_.extract_or_create_span(
utils::HeaderReader(*http_headers), span_cfg);
if (auto error = maybe_span.if_error()) {
logger_->log_error(
error->with_prefix("could not extract span from http_headers: "));
} else {
success(*maybe_span, res);
active_spans_.emplace(maybe_span->id(), std::move(*maybe_span));
return;
}
success(span, res);
active_spans_.emplace(span.id(), std::move(span));
return;
}
}

Expand Down
6 changes: 2 additions & 4 deletions test/test_tracer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,7 @@ TEST_CASE("span extraction") {
const std::unordered_map<std::string, std::string> no_headers;
MockDictReader reader{no_headers};
auto span = tracer.extract_or_create_span(reader);
REQUIRE(span);
REQUIRE(!span->parent_id());
REQUIRE(!span.parent_id());
}

SECTION("extraction failures") {
Expand Down Expand Up @@ -546,8 +545,7 @@ TEST_CASE("span extraction") {
auto span = tracer.extract_or_create_span(reader);
auto method = "extract_or_create_span";
CAPTURE(method);
REQUIRE(span);
checks(test_case, *span);
checks(test_case, span);
}
}

Expand Down