-
Notifications
You must be signed in to change notification settings - Fork 383
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
Adding a dynamic dispatch to Aggregator Selector #497
Merged
jtescher
merged 13 commits into
open-telemetry:main
from
dawid-nowak:adding_dynamic_dispatch_to_aggregator_selector
Apr 7, 2021
Merged
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
2eb9af1
Adding a dynamic dispatch to Aggregator Selector
dawid-nowak c5cb66c
Keeping lint happy
dawid-nowak fde3cc0
Merge branch 'main' into adding_dynamic_dispatch_to_aggregator_selector
dawid-nowak f9791e9
Removing box from with_aggregator_selector
dawid-nowak 4b6351a
Merge branch 'adding_dynamic_dispatch_to_aggregator_selector' of http…
dawid-nowak 398d34b
Removig more boxes
dawid-nowak ffcf32a
Merge branch 'main' into adding_dynamic_dispatch_to_aggregator_selector
dawid-nowak 88b1904
with_aggregator_selector suggestions
jtescher ad5d801
Merge branch 'main' into adding_dynamic_dispatch_to_aggregator_selector
dawid-nowak 14051a2
Added histogram to custom aggregator in examples
dawid-nowak eff68f0
Merge branch 'adding_dynamic_dispatch_to_aggregator_selector' of http…
dawid-nowak 8446a76
Keeping linter happy
dawid-nowak 7f00b6a
Merge branch 'main' into adding_dynamic_dispatch_to_aggregator_selector
jtescher File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
[package] | ||
name = "basic-otlp-with-selector" | ||
version = "0.1.0" | ||
edition = "2018" | ||
|
||
[dependencies] | ||
futures = "0.3" | ||
lazy_static = "1.4" | ||
opentelemetry = { path = "../../opentelemetry", features = ["rt-tokio", "metrics", "serialize"] } | ||
opentelemetry-otlp = { path = "../../opentelemetry-otlp", features = ["tonic", "metrics"] } | ||
serde_json = "1.0" | ||
tokio = { version = "1.0", features = ["full"] } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
# Basic OpenTelemetry Example | ||
|
||
This example shows basic span and metric usage, and exports to the [OpenTelemetry Collector](https://github.com/open-telemetry/opentelemetry-collector) via OTLP with a custom metric aggregator selector. | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,131 @@ | ||
use futures::stream::Stream; | ||
use futures::StreamExt; | ||
use opentelemetry::global::shutdown_tracer_provider; | ||
use opentelemetry::sdk::{ | ||
export::metrics::{Aggregator, AggregatorSelector}, | ||
metrics::{aggregators, PushController}, | ||
}; | ||
use opentelemetry::trace::TraceError; | ||
use opentelemetry::{ | ||
baggage::BaggageExt, | ||
metrics::{self, Descriptor, ObserverResult}, | ||
trace::{TraceContextExt, Tracer}, | ||
Context, Key, KeyValue, | ||
}; | ||
use opentelemetry::{global, sdk::trace as sdktrace}; | ||
use opentelemetry_otlp::ExporterConfig; | ||
use opentelemetry_otlp::Protocol; | ||
use std::error::Error; | ||
use std::sync::Arc; | ||
use std::time::Duration; | ||
|
||
fn init_tracer() -> Result<sdktrace::Tracer, TraceError> { | ||
opentelemetry_otlp::new_pipeline() | ||
.with_endpoint("http://localhost:4317") | ||
.with_tonic() | ||
.install_batch(opentelemetry::runtime::Tokio) | ||
} | ||
|
||
// Skip first immediate tick from tokio, not needed for async_std. | ||
fn delayed_interval(duration: Duration) -> impl Stream<Item = tokio::time::Instant> { | ||
opentelemetry::util::tokio_interval_stream(duration).skip(1) | ||
} | ||
|
||
#[derive(Debug)] | ||
struct CustomAggregator(); | ||
|
||
impl AggregatorSelector for CustomAggregator { | ||
fn aggregator_for( | ||
&self, | ||
descriptor: &Descriptor, | ||
) -> Option<Arc<(dyn Aggregator + Sync + std::marker::Send + 'static)>> { | ||
match descriptor.name() { | ||
"ex.com.one" => Some(Arc::new(aggregators::last_value())), | ||
"ex.com.two" => Some(Arc::new(aggregators::array())), | ||
_ => Some(Arc::new(aggregators::sum())), | ||
} | ||
} | ||
} | ||
|
||
fn init_meter() -> metrics::Result<PushController> { | ||
let export_config = ExporterConfig { | ||
endpoint: "http://localhost:4317".to_string(), | ||
protocol: Protocol::Grpc, | ||
..ExporterConfig::default() | ||
}; | ||
opentelemetry_otlp::new_metrics_pipeline(tokio::spawn, delayed_interval) | ||
.with_export_config(export_config) | ||
.with_aggregator_selector(CustomAggregator()) | ||
.build() | ||
} | ||
|
||
const FOO_KEY: Key = Key::from_static_str("ex.com/foo"); | ||
const BAR_KEY: Key = Key::from_static_str("ex.com/bar"); | ||
const LEMONS_KEY: Key = Key::from_static_str("lemons"); | ||
const ANOTHER_KEY: Key = Key::from_static_str("ex.com/another"); | ||
|
||
lazy_static::lazy_static! { | ||
static ref COMMON_LABELS: [KeyValue; 4] = [ | ||
LEMONS_KEY.i64(10), | ||
KeyValue::new("A", "1"), | ||
KeyValue::new("B", "2"), | ||
KeyValue::new("C", "3"), | ||
]; | ||
} | ||
|
||
#[tokio::main] | ||
async fn main() -> Result<(), Box<dyn Error + Send + Sync + 'static>> { | ||
let _ = init_tracer()?; | ||
let _started = init_meter()?; | ||
|
||
let tracer = global::tracer("ex.com/basic"); | ||
let meter = global::meter("ex.com/basic"); | ||
|
||
let one_metric_callback = |res: ObserverResult<f64>| res.observe(1.0, COMMON_LABELS.as_ref()); | ||
let _ = meter | ||
.f64_value_observer("ex.com.one", one_metric_callback) | ||
.with_description("A ValueObserver set to 1.0") | ||
.init(); | ||
|
||
let value_recorder_two = meter.f64_value_recorder("ex.com.two").init(); | ||
|
||
let another_recorder = meter.f64_value_recorder("ex.com.two").init(); | ||
another_recorder.record(5.5, COMMON_LABELS.as_ref()); | ||
|
||
let _baggage = | ||
Context::current_with_baggage(vec![FOO_KEY.string("foo1"), BAR_KEY.string("bar1")]) | ||
.attach(); | ||
|
||
let value_recorder = value_recorder_two.bind(COMMON_LABELS.as_ref()); | ||
tracer.in_span("operation", |cx| { | ||
let span = cx.span(); | ||
span.add_event( | ||
"Nice operation!".to_string(), | ||
vec![Key::new("bogons").i64(100)], | ||
); | ||
span.set_attribute(ANOTHER_KEY.string("yes")); | ||
|
||
meter.record_batch_with_context( | ||
// Note: call-site variables added as context Entries: | ||
&Context::current_with_baggage(vec![ANOTHER_KEY.string("xyz")]), | ||
COMMON_LABELS.as_ref(), | ||
vec![value_recorder_two.measurement(2.0)], | ||
); | ||
|
||
tracer.in_span("Sub operation...", |cx| { | ||
let span = cx.span(); | ||
span.set_attribute(LEMONS_KEY.string("five")); | ||
|
||
span.add_event("Sub span event".to_string(), vec![]); | ||
|
||
value_recorder.record(1.3); | ||
}); | ||
}); | ||
|
||
// wait for 1 minutes so that we could see metrics being pushed via OTLP every 10 seconds. | ||
tokio::time::sleep(Duration::from_secs(60)).await; | ||
|
||
shutdown_tracer_provider(); | ||
|
||
Ok(()) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Are all these other box changes still needed?
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.
I think so. I tried it before and was running into problems. If my understanding is correct if you remove Box::dyn then the compiler does static dispatch and turns T into a concrete type. So without dyns, it works in a situation where both
opentelemetry_otlp::new_metrics_pipeline(tokio::spawn, delayed_interval)
and.with_aggregator_selector()
use the same type T.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.
If you replace the method body with the full:
Then you don't need any other change.
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.
I have a feeling I am missing something here.
If I use the snippet from above I am getting compilation errors on both the main and this branch.
If I remove
Box::dyns
I am pretty much back to the code that is currently on the main branch. In there the aggregator selector innew_metrics_pipeline
must be of the same type as inwith_aggreagtor_selector
.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.
i have removed more dyns from
new_metrics_pipeline_with_selector
but i am not sure if i can remove the remaining onesThere 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.
Something along the lines of 88b1904 should work
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.
@jtescher I have missed that. The code is much simpler indeed, thanks. I have merged your suggestions, so hopefully, we are good now.