From 4ae02752bf74d9b3067cdabcbe9052a98bc8cada Mon Sep 17 00:00:00 2001 From: Julian Tescher Date: Mon, 23 Mar 2020 21:27:58 -0700 Subject: [PATCH] Clean up Span and Event APIs (#80) This updates the Span API to reflect the fact that data access is already synchronized behind a mutex so the additional `&mut` restrictions are unnecessary. It also renames event's `message` to `name` to better reflect the specification. --- benches/trace.rs | 8 ++++---- examples/report.rs | 2 +- opentelemetry-jaeger/src/lib.rs | 4 ++-- opentelemetry-zipkin/src/lib.rs | 2 +- src/api/trace/event.rs | 14 +++++++------- src/api/trace/noop.rs | 12 ++++++------ src/api/trace/span.rs | 14 +++++++------- src/api/trace/tracer.rs | 8 ++++---- src/global.rs | 26 ++++++++++++++++++-------- src/sdk/trace/span.rs | 24 +++++++++++++++++------- 10 files changed, 67 insertions(+), 47 deletions(-) diff --git a/benches/trace.rs b/benches/trace.rs index 006563eef8..161cfe6c40 100644 --- a/benches/trace.rs +++ b/benches/trace.rs @@ -10,7 +10,7 @@ fn criterion_benchmark(c: &mut Criterion) { }); trace_benchmark_group(c, "start-end-span-4-attrs", |tracer| { - let mut span = tracer.start("foo", None); + let span = tracer.start("foo", None); span.set_attribute(Key::new("key1").bool(false)); span.set_attribute(Key::new("key2").string("hello")); span.set_attribute(Key::new("key3").u64(123)); @@ -19,7 +19,7 @@ fn criterion_benchmark(c: &mut Criterion) { }); trace_benchmark_group(c, "start-end-span-8-attrs", |tracer| { - let mut span = tracer.start("foo", None); + let span = tracer.start("foo", None); span.set_attribute(Key::new("key1").bool(false)); span.set_attribute(Key::new("key2").string("hello")); span.set_attribute(Key::new("key3").u64(123)); @@ -32,7 +32,7 @@ fn criterion_benchmark(c: &mut Criterion) { }); trace_benchmark_group(c, "start-end-span-all-attr-types", |tracer| { - let mut span = tracer.start("foo", None); + let span = tracer.start("foo", None); span.set_attribute(Key::new("key1").bool(false)); span.set_attribute(Key::new("key2").string("hello")); span.set_attribute(Key::new("key3").i64(123)); @@ -45,7 +45,7 @@ fn criterion_benchmark(c: &mut Criterion) { }); trace_benchmark_group(c, "start-end-span-all-attr-types-2x", |tracer| { - let mut span = tracer.start("foo", None); + let span = tracer.start("foo", None); span.set_attribute(Key::new("key1").bool(false)); span.set_attribute(Key::new("key2").string("hello")); span.set_attribute(Key::new("key3").i64(123)); diff --git a/examples/report.rs b/examples/report.rs index 1815d20a58..d578ff424b 100644 --- a/examples/report.rs +++ b/examples/report.rs @@ -13,7 +13,7 @@ fn main() { let span0 = tracer.start("main", None); thread::sleep(Duration::from_millis(10)); { - let mut span1 = tracer.start("sub", Some(span0.get_context())); + let span1 = tracer.start("sub", Some(span0.get_context())); span1.set_attribute(api::Key::new("foo").string("bar")); span1.add_event("something wrong".to_string()); thread::sleep(Duration::from_millis(10)); diff --git a/opentelemetry-jaeger/src/lib.rs b/opentelemetry-jaeger/src/lib.rs index 4b4818c657..e29284f402 100644 --- a/opentelemetry-jaeger/src/lib.rs +++ b/opentelemetry-jaeger/src/lib.rs @@ -317,9 +317,9 @@ impl Into for api::Event { jaeger::Log::new( timestamp, vec![jaeger::Tag::new( - "event".to_string(), + "name".to_string(), jaeger::TagType::String, - Some(self.message), + Some(self.name), None, None, None, diff --git a/opentelemetry-zipkin/src/lib.rs b/opentelemetry-zipkin/src/lib.rs index f4d74a23ba..47b619d3d8 100644 --- a/opentelemetry-zipkin/src/lib.rs +++ b/opentelemetry-zipkin/src/lib.rs @@ -187,7 +187,7 @@ impl Into for api::Event { annotation::Annotation::builder() .timestamp(timestamp) - .value(self.message) + .value(self.name) .build() } } diff --git a/src/api/trace/event.rs b/src/api/trace/event.rs index 09e6f95427..6d420cb4dc 100644 --- a/src/api/trace/event.rs +++ b/src/api/trace/event.rs @@ -9,22 +9,22 @@ use std::time::SystemTime; #[cfg_attr(feature = "serialize", derive(Deserialize, PartialEq, Serialize))] #[derive(Clone, Debug)] pub struct Event { - /// Event message - pub message: String, + /// Event name + pub name: String, /// Event timestamp pub timestamp: SystemTime, } impl Event { /// Create new `Event` - pub fn new(message: String, timestamp: SystemTime) -> Self { - Event { message, timestamp } + pub fn new(name: String, timestamp: SystemTime) -> Self { + Event { name, timestamp } } - /// Create new `Event` for a given message. - pub fn from_message(message: String) -> Self { + /// Create new `Event` with a given name. + pub fn with_name(name: String) -> Self { Event { - message, + name, timestamp: SystemTime::now(), } } diff --git a/src/api/trace/noop.rs b/src/api/trace/noop.rs index ca8d7f956f..ee255cfeb3 100644 --- a/src/api/trace/noop.rs +++ b/src/api/trace/noop.rs @@ -48,12 +48,12 @@ impl NoopSpan { impl api::Span for NoopSpan { /// Ignores all events - fn add_event(&mut self, _message: String) { + fn add_event(&self, _name: String) { // Ignore } /// Ignores all events with timestamps - fn add_event_with_timestamp(&mut self, _message: String, _timestamp: SystemTime) { + fn add_event_with_timestamp(&self, _name: String, _timestamp: SystemTime) { // Ignored } @@ -68,22 +68,22 @@ impl api::Span for NoopSpan { } /// Ignores all attributes - fn set_attribute(&mut self, _attribute: api::KeyValue) { + fn set_attribute(&self, _attribute: api::KeyValue) { // Ignored } /// Ignores status - fn set_status(&mut self, _status: api::SpanStatus) { + fn set_status(&self, _status: api::SpanStatus) { // Ignored } /// Ignores name updates - fn update_name(&mut self, _new_name: String) { + fn update_name(&self, _new_name: String) { // Ignored } /// Ignores `Span` endings. - fn end(&mut self) { + fn end(&self) { // Ignored } diff --git a/src/api/trace/span.rs b/src/api/trace/span.rs index dd3f36cd25..49d0ce377a 100644 --- a/src/api/trace/span.rs +++ b/src/api/trace/span.rs @@ -34,8 +34,8 @@ pub trait Span: fmt::Debug + 'static { /// Note that the OpenTelemetry project documents certain ["standard event names and /// keys"](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md) /// which have prescribed semantic meanings. - fn add_event(&mut self, message: String) { - self.add_event_with_timestamp(message, SystemTime::now()) + fn add_event(&self, name: String) { + self.add_event_with_timestamp(name, SystemTime::now()) } /// An API to record events at a specific time in the context of a given `Span`. @@ -46,7 +46,7 @@ pub trait Span: fmt::Debug + 'static { /// Note that the OpenTelemetry project documents certain ["standard event names and /// keys"](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md) /// which have prescribed semantic meanings. - fn add_event_with_timestamp(&mut self, message: String, timestamp: SystemTime); + fn add_event_with_timestamp(&self, name: String, timestamp: SystemTime); /// Returns the `SpanContext` for the given `Span`. The returned value may be used even after /// the `Span is finished. The returned value MUST be the same for the entire `Span` lifetime. @@ -83,14 +83,14 @@ pub trait Span: fmt::Debug + 'static { /// Note that the OpenTelemetry project documents certain ["standard /// attributes"](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md) /// that have prescribed semantic meanings. - fn set_attribute(&mut self, attribute: api::KeyValue); + fn set_attribute(&self, attribute: api::KeyValue); /// Sets the status of the `Span`. If used, this will override the default `Span` /// status, which is `OK`. /// /// Only the value of the last call will be recorded, and implementations are free /// to ignore previous calls. - fn set_status(&mut self, status: api::SpanStatus); + fn set_status(&self, status: api::SpanStatus); /// Updates the `Span`'s name. After this update, any sampling behavior based on the /// name will depend on the implementation. @@ -104,7 +104,7 @@ pub trait Span: fmt::Debug + 'static { /// regular property. It emphasizes that this operation signifies a /// major change for a `Span` and may lead to re-calculation of sampling or /// filtering decisions made previously depending on the implementation. - fn update_name(&mut self, new_name: String); + fn update_name(&self, new_name: String); /// Finishes the `Span`. /// @@ -116,7 +116,7 @@ pub trait Span: fmt::Debug + 'static { /// still be running and can be ended later. /// ///This API MUST be non-blocking. - fn end(&mut self); + fn end(&self); /// Used by global tracer to downcast to specific span type. fn as_any(&self) -> &dyn std::any::Any; diff --git a/src/api/trace/tracer.rs b/src/api/trace/tracer.rs index e434698ab8..72d3ff8d57 100644 --- a/src/api/trace/tracer.rs +++ b/src/api/trace/tracer.rs @@ -112,7 +112,7 @@ pub trait TracerGenerics: Tracer { /// It then executes the body. It closes the span before returning the execution result. fn with_span(&self, name: &'static str, f: F) -> T where - F: FnOnce(&mut Self::Span) -> T; + F: FnOnce(&Self::Span) -> T; } // These functions can be implemented for all tracers to allow for convenient `with_span` syntax. @@ -122,12 +122,12 @@ impl TracerGenerics for S { /// It then executes the body. It closes the span before returning the execution result. fn with_span(&self, name: &'static str, f: F) -> T where - F: FnOnce(&mut Self::Span) -> T, + F: FnOnce(&Self::Span) -> T, { - let mut span = self.start(name, None); + let span = self.start(name, None); self.mark_span_as_active(&span); - let result = f(&mut span); + let result = f(&span); span.end(); self.mark_span_as_inactive(span.get_context().span_id()); diff --git a/src/global.rs b/src/global.rs index 425dd08ede..10376ffa74 100644 --- a/src/global.rs +++ b/src/global.rs @@ -82,8 +82,8 @@ impl api::Span for BoxedSpan { /// Note that the OpenTelemetry project documents certain ["standard event names and /// keys"](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md) /// which have prescribed semantic meanings. - fn add_event_with_timestamp(&mut self, message: String, timestamp: SystemTime) { - self.0.add_event_with_timestamp(message, timestamp) + fn add_event_with_timestamp(&self, name: String, timestamp: SystemTime) { + self.0.add_event_with_timestamp(name, timestamp) } /// Returns the `SpanContext` for the given `Span`. @@ -102,23 +102,23 @@ impl api::Span for BoxedSpan { /// Note that the OpenTelemetry project documents certain ["standard /// attributes"](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md) /// that have prescribed semantic meanings. - fn set_attribute(&mut self, attribute: api::KeyValue) { + fn set_attribute(&self, attribute: api::KeyValue) { self.0.set_attribute(attribute) } /// Sets the status of the `Span`. If used, this will override the default `Span` /// status, which is `OK`. - fn set_status(&mut self, status: api::SpanStatus) { + fn set_status(&self, status: api::SpanStatus) { self.0.set_status(status) } /// Updates the `Span`'s name. - fn update_name(&mut self, new_name: String) { + fn update_name(&self, new_name: String) { self.0.update_name(new_name) } /// Finishes the span. - fn end(&mut self) { + fn end(&self) { self.0.end() } @@ -127,12 +127,22 @@ impl api::Span for BoxedSpan { self } - /// Mark span as active + /// Mark span as currently active + /// + /// This is the _synchronous_ api. If you are using futures, you + /// need to use the async api via [`instrument`]. + /// + /// [`instrument`]: ../api/trace/futures/trait.Instrument.html#method.instrument fn mark_as_active(&self) { self.0.mark_as_active() } - /// Mark span as inactive + /// Mark span as no longer active + /// + /// This is the _synchronous_ api. If you are using futures, you + /// need to use the async api via [`instrument`]. + /// + /// [`instrument`]: ../api/trace/futures/trait.Instrument.html#method.instrument fn mark_as_inactive(&self) { self.0.mark_as_inactive() } diff --git a/src/sdk/trace/span.rs b/src/sdk/trace/span.rs index d2a1204678..0bf6d773fa 100644 --- a/src/sdk/trace/span.rs +++ b/src/sdk/trace/span.rs @@ -77,10 +77,10 @@ impl api::Span for Span { /// Note that the OpenTelemetry project documents certain ["standard event names and /// keys"](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md) /// which have prescribed semantic meanings. - fn add_event_with_timestamp(&mut self, message: String, timestamp: SystemTime) { + fn add_event_with_timestamp(&self, name: String, timestamp: SystemTime) { self.with_data_mut(|data| { data.message_events - .push_front(api::Event { message, timestamp }) + .push_front(api::Event { name, timestamp }) }); } @@ -103,7 +103,7 @@ impl api::Span for Span { /// Note that the OpenTelemetry project documents certain ["standard /// attributes"](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md) /// that have prescribed semantic meanings. - fn set_attribute(&mut self, attribute: api::KeyValue) { + fn set_attribute(&self, attribute: api::KeyValue) { self.with_data_mut(|data| { data.attributes.push_front(attribute); }); @@ -111,21 +111,21 @@ impl api::Span for Span { /// Sets the status of the `Span`. If used, this will override the default `Span` /// status, which is `OK`. - fn set_status(&mut self, status: api::SpanStatus) { + fn set_status(&self, status: api::SpanStatus) { self.with_data_mut(|data| { data.status = status; }); } /// Updates the `Span`'s name. - fn update_name(&mut self, new_name: String) { + fn update_name(&self, new_name: String) { self.with_data_mut(|data| { data.name = new_name; }); } /// Finishes the span. - fn end(&mut self) { + fn end(&self) { self.with_data_mut(|data| { data.end_time = SystemTime::now(); }); @@ -136,12 +136,22 @@ impl api::Span for Span { self } - /// Mark span as active + /// Mark as currently active span. + /// + /// This is the _synchronous_ api. If you are using futures, you + /// need to use the async api via [`instrument`]. + /// + /// [`instrument`]: ../../api/trace/futures/trait.Instrument.html#method.instrument fn mark_as_active(&self) { self.inner.tracer.mark_span_as_active(&self); } /// Mark span as inactive + /// + /// This is the _synchronous_ api. If you are using futures, you + /// need to use the async api via [`instrument`]. + /// + /// [`instrument`]: ../futures/trait.Instrument.html#method.instrument fn mark_as_inactive(&self) { self.inner.tracer.mark_span_as_inactive(self.id); }