-
Notifications
You must be signed in to change notification settings - Fork 34
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
OTLP registry emitter #549
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #549 +/- ##
=====================================
Coverage 74.6% 74.6%
=====================================
Files 51 51
Lines 3965 3965
=====================================
+ Hits 2958 2959 +1
+ Misses 1007 1006 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I really like the idea of having a semconv registry-driven generation of OTLP signals! It opens up a lot of interesting possibilities. An immediate use case is the ability to use semconv as a mechanism to decouple the development of an application from the development of the observability infrastructure associated with that application (e.g., creating dashboards even before the application is developed). Telemetry-schema driven simulations is another long-term option :-)
I have a few requests for minor changes, and I would also like to get @jsuereth 's feedback on this PR.
Thanks again, this is really great!
/// Write the telemetry to standard output | ||
#[arg(long)] | ||
stdout: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the environment variable, I would find it more “discoverable” to have a parameter (or two) allowing the specification of the gRPC endpoint to target.
|
||
// For the given attribute, return a name/value pair. | ||
// Values are generated based on the attribute type and examples where possible. | ||
fn get_attribute_name_value(attribute: &Attribute) -> KeyValue { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Supporting additional signals will significantly increase the number of methods used for generation and, consequently, the size of this file. Since we don’t know who will implement support for the other signals, I think it would be a good idea to create a new crate (e.g., weaver_emit) in /crates
so that this current file contains only the essentials for defining the command interface, loading the registry, and calling the signal generation method.
fn init_tracer_provider() -> Result<sdktrace::TracerProvider, TraceError> { | ||
let exporter = opentelemetry_otlp::SpanExporter::builder() | ||
.with_tonic() | ||
.with_endpoint("http://localhost:4317") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use the default of the corresponding command parameter
let rt = tokio::runtime::Runtime::new().expect("failed to create tokio runtime"); | ||
rt.block_on(async { | ||
let tracer_provider = if args.stdout { | ||
logger.mute(); | ||
init_stdout_tracer_provider() | ||
} else { | ||
init_tracer_provider().expect("OTLP Tracer Provider must be created") | ||
}; | ||
let _ = global::set_tracer_provider(tracer_provider.clone()); | ||
|
||
emit_trace_for_registry(®istry, &args.registry.registry.to_string()); | ||
|
||
global::shutdown_tracer_provider(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the conversation with @jsuereth this morning, I think the creation of the Tokio runtime should be encapsulated in the weaver_emit
crate mentioned earlier to avoid any async aspects at this level (even though it is honestly quite limited).
Totally agree with your comments @lquerel, and will go ahead with those changes. Thanks! |
First iteration of the new command:
registry emit
. Emits a semantic convention registry as example spans to your OTLP receiver. This may be useful in testing/simulation scenarios. Defaults tohttp://localhost:4317
if the OpenTelemetry standardOTEL_EXPORTER_OTLP_ENDPOINT
env var is unset.With this command:
A single trace with all defined spans in the current Semantic Conventions are sent to the receiver:
Spans:
span_kind
A future PR will cover:
conditionally_required
clause (programmatically doing this, is not possible)