chore: Remove Atoms #4485

Hoverbear · 2020-10-09T22:30:33Z

A component of https://github.com/timberio/vector/blob/c4707947bd876a0ff7d7aa36717ae2b32b731593/rfcs/2020-05-25-more-usable-logevents.md#sales-pitch

Precursor of #2843

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Hoverbear · 2020-10-13T19:48:07Z

src/event/log_event.rs

+    #[instrument(skip(self, key), fields(key = %key.as_ref()))]
+    pub fn insert(
+        &mut self,
+        key: impl AsRef<str>,


Future work we should probably upgrade this to an owned string so we can insert that allocation.

Hoverbear · 2020-10-13T19:50:47Z

src/internal_events/elasticsearch.rs

@@ -21,7 +20,7 @@ impl InternalEvent for ElasticSearchEventReceived {

 #[derive(Debug)]
 pub struct ElasticSearchMissingKeys {
-    pub keys: Vec<Atom>,
+    pub keys: Vec<String>,


The Event Driven Observability RFC suggests we do not use Vec<String> here, and a &[&str]] would be better.

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Hoverbear · 2020-10-13T20:49:21Z

src/internal_events/elasticsearch.rs

-pub struct ElasticSearchMissingKeys {
-    pub keys: Vec<Atom>,
+pub struct ElasticSearchMissingKeys<'a> {
+    pub keys: &'a [String],


Allowing this instead of a &'a [&'a str] removes the need for the code that calls this to make an extra allocation through a by_ref() call.

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Hoverbear · 2020-10-13T21:23:33Z

src/sources/docker.rs

-    static ref NAME: Atom = Atom::from("container_name");
-    static ref STREAM: Atom = Atom::from("stream");
-    static ref CONTAINER: Atom = Atom::from("container_id");
+    static ref IMAGE: &'static str = "image";


These should be const

Signed-off-by: Ana Hobden <operator@hoverbear.org>

jszwedko

Awesome!

I focused on src/event and other non-component or internal events files and just scanned the rest since it seemed to be mechanical.

Cargo.toml

benches/lua.rs

jszwedko · 2020-10-13T21:29:25Z

src/event/log_event.rs


 #[derive(PartialEq, Debug, Clone, Default)]
 pub struct LogEvent {
    fields: BTreeMap<String, Value>,
 }

 impl LogEvent {
-    pub fn get(&self, key: &DefaultAtom) -> Option<&Value> {
-        util::log::get(&self.fields, key)
+    #[instrument(skip(self, key), fields(key = %key.as_ref()))]


I see you've been adding these since the introduction of the Lookup type. Do you recommend we add them to all methods? Or just certain ones? They seem useful for tracing.

We haven't really made an architectural call one way or the other. What they do is populate the span: https://tracing-rs.netlify.app/tracing/attr.instrument.html, so if we did wide adoption we might be able to reap some really nice contextual rewards. It's a practice I've been doing in my own code lately. I'm not sure if @lukesteensen has an opinion though.

I've seen these used as well.

I agree that better instrumentation is a worthy goal, but I would prefer if we at least document how/why we use them, including some best-practices/patterns everyone should follow.

I don't mind if it's in an RFC or a "tracking issue" that contains some context, and a few relevant links.

better instrumentation is a worthy goal, but I would prefer if we at least document how/why we use them, including some best-practices/patterns everyone should follow.

I agree! I'd also note that we should strive to be as consistent as possible across the codebase. The macro is cool and useful, but right now it mostly tells me which code was written by @Hoverbear 😄

An easy first step would be to open an issue for setting some guidelines around thing kind of instrumentation. We'll want to cover #[instrumented(...)] vs trace! vs emit!, message style, etc in the eventual guidelines, but for now, maybe @Hoverbear can just summarize her current thoughts as a starting point for the conversation.

I'd love to! For now, as a note #[instrument] just creates a span when the function is entered, with the function args (or as configured) as span items. It's dropped when the function exits. That's all it does. :)

It doesn't log. :)

src/mapping/query/path.rs

src/transforms/log_to_metric.rs

Signed-off-by: Ana Hobden <operator@hoverbear.org>

bruceg

Looks mostly ok, but I added a number of questions.

I am also concerned that, if we are serious about moving to either smolstr or smartstring for keys, we will end up reverting most of these changes and converting all Atom to SmartString or whatever.

src/event/discriminant.rs

bruceg · 2020-10-13T23:34:51Z

src/event/log_event.rs

+    #[instrument(skip(self, key), fields(key = %key.as_ref()))]
+    pub fn insert(
+        &mut self,
+        key: impl AsRef<str>,


src/event/log_event.rs

src/sinks/influxdb/metrics.rs

src/sinks/logdna.rs

src/sinks/splunk_hec.rs

src/sources/journald.rs

src/sources/kubernetes_logs/parser/cri.rs

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Hoverbear · 2020-10-14T15:03:30Z

@bruceg I feel if we move to smolstr or otherwise, we'll be able to internalize this decision making to the Value or Lookup type, or add transparent conversions, and not require such intrusive changes.

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Instrument eventlog Signed-off-by: Ana Hobden <operator@hoverbear.org> * Migrate get Signed-off-by: Ana Hobden <operator@hoverbear.org> * Migrate get_mut Signed-off-by: Ana Hobden <operator@hoverbear.org> * insert and try_insert migration Signed-off-by: Ana Hobden <operator@hoverbear.org> * Migrate remove Signed-off-by: Ana Hobden <operator@hoverbear.org> * wip Signed-off-by: Ana Hobden <operator@hoverbear.org> * Remove atom Signed-off-by: Ana Hobden <operator@hoverbear.org> * fmt Signed-off-by: Ana Hobden <operator@hoverbear.org> * Fixup Signed-off-by: Ana Hobden <operator@hoverbear.org> * Fixups Signed-off-by: Ana Hobden <operator@hoverbear.org> * Fixups Signed-off-by: Ana Hobden <operator@hoverbear.org> * Internal event fixups Signed-off-by: Ana Hobden <operator@hoverbear.org> * remove a comment Signed-off-by: Ana Hobden <operator@hoverbear.org> * Fixup some consts Signed-off-by: Ana Hobden <operator@hoverbear.org> * Fixups Signed-off-by: Ana Hobden <operator@hoverbear.org> * fixup trailing Signed-off-by: Ana Hobden <operator@hoverbear.org> * chore: Fixup Cargo deny Signed-off-by: Ana Hobden <operator@hoverbear.org> * Fixups Signed-off-by: Ana Hobden <operator@hoverbear.org> * fmt Signed-off-by: Ana Hobden <operator@hoverbear.org> Signed-off-by: Brian Menges <brian.menges@anaplan.com>

Hoverbear added 10 commits October 9, 2020 09:03

Instrument eventlog

7458356

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Migrate get

80c2878

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Migrate get_mut

779f96c

Signed-off-by: Ana Hobden <operator@hoverbear.org>

insert and try_insert migration

5e247e6

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Migrate remove

78e893b

Signed-off-by: Ana Hobden <operator@hoverbear.org>

wip

cdaee53

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Remove atom

65788b0

Signed-off-by: Ana Hobden <operator@hoverbear.org>

fmt

61aafdd

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Merge branch 'master' into atomless

4bf04ef

Fixup

c525e29

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Hoverbear force-pushed the atomless branch from b5f6756 to c525e29 Compare October 13, 2020 19:28

Hoverbear changed the title ~~Remove Atoms~~ chore: Remove Atoms Oct 13, 2020

Hoverbear requested a review from jszwedko October 13, 2020 19:28

Hoverbear marked this pull request as ready for review October 13, 2020 19:28

Hoverbear requested review from bruceg, fanatid, StephenWakely, JeanMertz, ktff, lukesteensen and MOZGIII as code owners October 13, 2020 19:28

Hoverbear removed request for StephenWakely, MOZGIII, lukesteensen, JeanMertz, bruceg, fanatid and ktff October 13, 2020 19:29

Hoverbear added this to the 2020-10-12: Son of Flynn milestone Oct 13, 2020

Hoverbear added the domain: logs Anything related to Vector's log events label Oct 13, 2020

Hoverbear added the domain: data model Anything related to Vector's internal data model label Oct 13, 2020

Hoverbear self-assigned this Oct 13, 2020

Hoverbear commented Oct 13, 2020

View reviewed changes

Hoverbear added 2 commits October 13, 2020 13:02

Fixups

329f25e

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Fixups

e2f0101

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Hoverbear commented Oct 13, 2020

View reviewed changes

Hoverbear added 2 commits October 13, 2020 13:58

Internal event fixups

9765c9f

Signed-off-by: Ana Hobden <operator@hoverbear.org>

remove a comment

8c0829f

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Hoverbear commented Oct 13, 2020

View reviewed changes

Fixup some consts

8e1bd62

Signed-off-by: Ana Hobden <operator@hoverbear.org>

jszwedko approved these changes Oct 13, 2020

View reviewed changes

Hoverbear added 3 commits October 13, 2020 15:03

Fixups

fb0e982

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Merge branch 'master' into atomless

013f52a

fixup trailing

04dc150

Signed-off-by: Ana Hobden <operator@hoverbear.org>

bruceg reviewed Oct 14, 2020

View reviewed changes

Hoverbear added 2 commits October 14, 2020 07:56

chore: Fixup Cargo deny

95e0107

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Merge branch 'fixup-deny' into atomless

df4bdb4

Hoverbear added 2 commits October 14, 2020 08:31

Fixups

97343a8

Signed-off-by: Ana Hobden <operator@hoverbear.org>

fmt

362e7a5

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Hoverbear merged commit 549f695 into master Oct 14, 2020

Hoverbear deleted the atomless branch October 14, 2020 17:46

Hoverbear mentioned this pull request Oct 26, 2020

tracing::instrument decorator should require a level argument. tokio-rs/tracing#1070

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Remove Atoms #4485

chore: Remove Atoms #4485

Hoverbear commented Oct 9, 2020 •

edited

Loading

Hoverbear Oct 13, 2020

bruceg Oct 13, 2020

Hoverbear Oct 13, 2020

Hoverbear Oct 13, 2020

Hoverbear Oct 13, 2020

jszwedko left a comment

jszwedko Oct 13, 2020

Hoverbear Oct 13, 2020

JeanMertz Oct 14, 2020

lukesteensen Oct 14, 2020

Hoverbear Oct 14, 2020

bruceg left a comment

bruceg Oct 13, 2020

Hoverbear commented Oct 14, 2020

chore: Remove Atoms #4485

chore: Remove Atoms #4485

Conversation

Hoverbear commented Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jszwedko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruceg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hoverbear commented Oct 14, 2020

Hoverbear commented Oct 9, 2020 •

edited

Loading