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

feat(rust): Add Lightstep Adapter, generate Rust types based on otel protobufs #87

Merged
merged 12 commits into from
Aug 15, 2023

Conversation

wikiwong
Copy link
Contributor

  • Add Lightstep Adapter
  • Generate types based on protobuf definitions
  • Remove old otel formatter and replace with protobuf based formatter

@@ -15,7 +15,7 @@ use url::Url;

use crate::{Event, Log, Metric, TraceEvent};

use super::{
pub use super::{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub use super::{
use super::datadog_formatter::{DatadogFormatter, Span, Trace};
pub use super::{
Adapter, AdapterHandle, AdapterMetadata,
};

Copy link
Member

Choose a reason for hiding this comment

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

(not 100% sure about that particular suggestion -- only did on github so worth checking 😆)

Comment on lines 12 to 8
otel_formatter::{Attribute, Value},
AdapterMetadata,
otel_formatter::{opentelemetry, Attribute, OtelFormatter, Value},
Copy link
Member

Choose a reason for hiding this comment

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

Same suggestion as the Datadog public interface above; let's limit what we make public to only what we know the end-user needs.
This one probably should export the Attribute and Value in addition to the Adapter and related types, but not the formatter things.

Comment on lines 92 to 103
let mut span = OtelFormatter::new_span(
trace_id,
parent_id,
"allocation".to_string(),
a.ts,
a.ts,
);
OtelFormatter::add_attribute_i64_to_span(
&mut span,
"amount".to_string(),
a.amount.into(),
);
Copy link
Member

Choose a reason for hiding this comment

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

This is my bad (fixed in Go and JS, but i guess i forgot in Rust), but we need to fix this in all the adapters.. memory allocation events should pop the last function from the stack and add the allocation as an attribute to that, using the number of pages (the a.amount) as the value. Then, push that function back onto the stack and carry on

Comment on lines 91 to 109
Event::Alloc(a) => {
let mut span = OtelFormatter::new_span(
trace_id,
parent_id,
"allocation".to_string(),
a.ts,
a.ts,
);
OtelFormatter::add_attribute_i64_to_span(
&mut span,
"amount".to_string(),
a.amount.into(),
);
spans.push(span);
}
_ => {}
}
Ok(())
}
Copy link
Member

Choose a reason for hiding this comment

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

same comment re: attaching the allocation as an attribute to the previous function in the spans.

status: None,
events: vec![],
links: vec![],
trace_state: "".into(),
Copy link
Member

Choose a reason for hiding this comment

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

no need to update in the PR, but for future cases like this you can pass ..Default::default() as the last field in a struct literal as long as the Default trait is implemented. This will fill in the rest of the fields not explicitly included with their default values (like you've done manually).

Copy link
Member

@nilslice nilslice left a comment

Choose a reason for hiding this comment

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

LGTM!

@wikiwong wikiwong merged commit c846439 into main Aug 15, 2023
@wikiwong wikiwong deleted the lightstep-rust branch August 15, 2023 20:35
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.

2 participants