feat: glean metrics logic #1626

taddes · 2024-11-04T21:06:01Z

Description

Core Glean logic to process metrics and events.

Related to the PR presently in the glean_parser repo #772 for permanent rust server metric capabilities, so changes there may propagate to here

Testing

Issue(s)

Closes SYNC-4440.

glean/Cargo.toml

jrconlin · 2024-11-15T21:47:50Z

glean/src/server_events.rs

+    extra: HashMap<String, String>,
+}
+
+pub fn new_glean_event(


I'm a bit torn.

The more "rust"y way to do this would be to create an

impl Default for GleanEvent { fn default() -> Self { Self { category: String::default(), name: String::default(), timestamp: Utc::now().timestamp_millis(), extra: Hashmap::new(), } }

and then where you call new_glean_event you would specify it as:

GleanEvent { category, name, extra, ...Default::default() }

Generally, creating a ::new(...) is because there are more side effects or setup required than just a quick builder.

I'm not going to really push for that sort of change for something like this, though, so consider this more an FYI.

jrconlin · 2024-11-15T22:04:12Z

glean/src/server_events.rs

+}
+
+impl GleanEventsLogger {
+    fn create_client_info(&self) -> ClientInfo {


I would strongly recommend converting this to an impl Default unless there are more commonly used values to initialize this struct.

In discussing, and if ok with you, were gonna leave this one as it is closely tied to the GleanEventsLogger struct and needs values from the instance of said GleanEventsLogger, so wouldn't provide us with what we're looking for.

Ah, because the app_display_version and app_channel are defined from the values set in GleanEventsLogger.

Technically, a more rust way of doing that would be to create a

impl From<GleanEventsLogger> for ClientInfo { fn from(gel: GleanEventsLogger>) -> ClientInfo { ClientInfo { app_display_version: gel.app_display_version, app_channel: gel.app_channel, ..Default::default() } } }

(this is where you'd have a impl Default for ClientInfo that specifies the other values.)

You could then do

let telemetry_payload: PingPayload = PingPayload { client_info: self.clone().into(),

(This also lends itself nicely to template building, because then you can just define the non-standard stuff you want to specify and trust that rust will backfill with the standard bits.)

But if this is the way that the Glean team is advising you create this, so be it.

Sure but this also gets a bit in to the weeds of what you want for default values of app_display_version/channel -- I'm wondering if glean might never want those to be empty strings or "Unknown" (unlike the other fields) so you might want to discourage that from potentially ever happening by not having such a default as a footgun 🤷‍♂️

All the other parsers (JS, Go, Ruby, Python, etc) generate the string "Unknown" by default, so I think we want to keep it. The comment in one of the server implementations stated: "Unknown" fields below are required in the Glean schema, however they are not useful in server context. Therefore, we don't get any benefit but it is expected that the string "Unknown" is present.

glean/src/server_events.rs

jrconlin · 2024-11-15T22:08:05Z

glean/src/server_events.rs

+        payload: &PingPayload,
+    ) -> Ping {
+        let payload_json =
+            serde_json::to_string(payload).expect("unable to marshal payload to json.");


We should consider converting this to a thiserror derived struct so that it can be captured by calling systems and retried. An .expect() is a glorified panic and will kill whatever thread or process it's in.

I was ok with these as I can't imagine what could actually trigger the 2 expects generated in this file, maybe if you really screwed up your glean yaml/schema? In that case I'm ok with panic'ing -- and a nice side benefit is getting away without having to implement any actual Error stuff.

glean/src/server_events.rs

jrconlin · 2024-11-15T22:28:53Z

glean/src/server_events.rs

+        let mut metrics = Metrics::new();
+        // Create the inner metric value map to insert into `Metrics`.
+        metrics.insert(
+            "string".to_string(),


a bit nitty, but we generally call to_owned() instead of to_string(). Both are equally performant. to_owned() reflects that we're converting a static string reference (that's what "string" is) to a own-able String object. Since insert(...) essentially is consuming these values, it's semantically more correct.

syncserver/src/web/handlers.rs

pjenvey · 2024-11-18T23:44:25Z

syncserver/src/web/handlers.rs

+        app_id: "syncstorage".to_string(),
+        app_display_version: "1.0.0".to_string(),


Take a look at the logging module on what we use for its similar values. You probably want:

Suggested change

app_id: "syncstorage".to_string(),

app_display_version: "1.0.0".to_string(),

app_id: env!("CARGO_PKG_NAME").to_owned(),

app_display_version: env!("CARGO_PKG_VERSION").to_owned(),

app_id is actually what the application is defined as in the probe-scraper configuration: https://github.com/mozilla/probe-scraper/pull/834/files , but can use the app_display_version as you show above.

pjenvey · 2024-11-18T23:47:01Z

syncserver/src/web/handlers.rs

+        // app id will be supplied when added to probe-scraper
+        app_id: "syncstorage".to_string(),
+        app_display_version: "1.0.0".to_string(),
+        app_channel: "prod".to_string(),


We have a SENTRY_ENVIRONMENT env var which could be used here but we should probably just add a new SYNC_ENV var to the helm chart (like merino has).

Yeah, defining one that states it's the glean app_id is probably the way to go, eh? Can leave string for now while we wait till helm chart updated.

hardcoding the app_id to syncstorage wfm, I just mean something else for the app_channel of "prod"

Got the PR in place to add this, for now we will keep this in place and when the cloudops-infra changes land, we'll modify.

glean/metrics.yaml

Cargo.toml

jrconlin · 2024-11-19T23:00:04Z

glean/src/server_events.rs

+// Required imports
+use chrono::Utc;
+use serde::{Deserialize, Serialize};
+use serde_json;


Normally you don't have to specify crate roots like this. You only need to specify the various elements you're importing. Rust will automatically associate crates when you specify the root (like you do later for Value.)

If you want, you could specify

Suggested change

use serde_json;

use serde_json::{Value, to_string};

and then drop the crate prefix from those uses later.

(I'm surprised that clippy didn't yell about this.)

jrconlin · 2024-11-19T23:03:39Z

glean/src/server_events.rs

+}
+
+// Exported type for public method parameters
+// Default impl empty values will be omitted in json from ping struct definition


FWIW: serde offers a flag for this: https://serde.rs/field-attrs.html#skip_serializing_if

jrconlin · 2024-11-19T23:26:25Z

glean/src/server_events.rs

+}
+
+impl GleanEventsLogger {
+    fn create_client_info(&self) -> ClientInfo {


Ah, because the app_display_version and app_channel are defined from the values set in GleanEventsLogger.

Technically, a more rust way of doing that would be to create a

impl From<GleanEventsLogger> for ClientInfo { fn from(gel: GleanEventsLogger>) -> ClientInfo { ClientInfo { app_display_version: gel.app_display_version, app_channel: gel.app_channel, ..Default::default() } } }

(this is where you'd have a impl Default for ClientInfo that specifies the other values.)

You could then do

let telemetry_payload: PingPayload = PingPayload { client_info: self.clone().into(),

(This also lends itself nicely to template building, because then you can just define the non-standard stuff you want to specify and trust that rust will backfill with the standard bits.)

But if this is the way that the Glean team is advising you create this, so be it.

glean/src/server_events.rs

jrconlin · 2024-11-19T23:31:56Z

glean/src/server_events.rs

+    pub event: Option<Box<dyn EventsPingEvent>>, // valid event of `EventsPingEvent` for this ping.
+}
+
+// Record and submit `events` ping


We should make this a doc string, since we've got one for the record_events_ping() function.

jrconlin · 2024-11-19T23:32:51Z

glean/src/server_events.rs

+    pub syncstorage_platform: String, // Platform from which sync action was initiated. Firefox Desktop, Fenix, or Firefox iOS.
+}
+
+// Record and submit `sync-dau` ping


Again, probably should be a docstring

jrconlin · 2024-11-19T23:36:22Z

syncserver/src/server/mod.rs

@@ -266,6 +273,14 @@ impl Server {
            &Metrics::from(&metrics),
            blocking_threadpool.clone(),
        )?;
+        let glean_logger = Arc::new(GleanEventsLogger {


Future optimization would be to have glean_logger be an Option<GleanEventsLogger> and only instantiate it if settings.syncstorage.glean_enabled. That saves us a byte in the ServerState.

jrconlin · 2024-11-19T23:38:23Z

syncserver/src/web/handlers.rs

-        .and_then(|header| header.to_str().ok())
-        .unwrap_or("none");
-    let _device_info = get_device_info(user_agent);
+    if state.glean_enabled {


if you used Option<GleanEventLogger> you'd do something like:

if let Some(glean_logger) = state.glean_logger { ... glean_logger.record_events_ping( ...

…send_in_pings to metrics.yaml

…er fields as not needed

…orce check in get_collections

taddes · 2024-11-23T00:43:52Z

Made requested changes and really tightened up the documentation. As far as the From/Into impl stuff, I definitely want to implement it, but if ok with you we'll keep it the same as the generated Rust code for validating everything works and I'll open another short ticket for those refinements. Just makes it simple to validate our approach in case there's something standing out. Thanks!

taddes self-assigned this Nov 4, 2024

taddes force-pushed the feat/glean-metrics-module-SYNC-4440 branch from d779cc2 to 82a32fe Compare November 6, 2024 17:09

taddes changed the title ~~WIP: feat: glean metrics logic~~ feat: glean metrics logic Nov 12, 2024

taddes marked this pull request as ready for review November 12, 2024 20:02

taddes requested review from pjenvey and jrconlin November 13, 2024 21:01

taddes force-pushed the feat/glean-metrics-module-SYNC-4440 branch from fe603d9 to 28592aa Compare November 13, 2024 21:02

jrconlin requested changes Nov 15, 2024

View reviewed changes

pjenvey requested changes Nov 18, 2024

View reviewed changes

pjenvey reviewed Nov 19, 2024

View reviewed changes

glean/metrics.yaml Outdated Show resolved Hide resolved

taddes requested review from pjenvey and jrconlin November 19, 2024 22:48

jrconlin reviewed Nov 19, 2024

View reviewed changes

taddes added 17 commits November 22, 2024 17:41

update metrics uuid type to string, initial struct definitions

4d3d59f

methods for events and specific metrics

a297bef

add cargo.toml for glean crate

34a310b

create lib.rs for glean

6329236

add glean import to handlers

e2f69c4

add event logger

723cf66

add record_events_ping call

20725b7

update pings to define a glean-dau ping as well and add lifetime and …

bc6afc9

…send_in_pings to metrics.yaml

update to most recent version of glean parser code for server_events

634d465

update handler code to emit glean event

69bffc6

pass user agent string to RequestInfo struct

a2c793f

empty string for ip addr

a94647c

update official app id and change ip to empty string

61c44c6

collection refactor for server events ping types

c7880c0

mozlog compliant Type and Field, remove LogEvelope timestamp and logg…

d23bff8

…er fields as not needed

move uuid as workspace dep

1371982

change to_string instances to to_owned where better

524f263

taddes added 13 commits November 22, 2024 17:41

update server events types and add glean logger to ServerState

ddf80a8

wrap glean_logger in Arc, reference ServerState in get_collections

1d5963f

add glean_enabled to syncstorage settings, add to ServerState and enf…

978db1c

…orce check in get_collections

add fiends to extractors references to ServerState

a98a8fe

move variables into Ping struct

ed70257

missing refs in extractors and test for syncserver

73b55d1

variance between local and ci clippy

ab592f8

remove unused import

6517bc1

add GleanEventsLogger to test

a2192a8

update notification emails to sync-backend team

335e8d0

update uuid

d3c54ff

r add additional docstrings and context comments

6c012c5

review comments and doc updates

cfc1e53

taddes force-pushed the feat/glean-metrics-module-SYNC-4440 branch from fa2f0a8 to cfc1e53 Compare November 23, 2024 00:41

taddes requested a review from jrconlin November 23, 2024 00:41

jrconlin approved these changes Nov 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: glean metrics logic #1626

feat: glean metrics logic #1626

taddes commented Nov 4, 2024 •

edited

Loading

jrconlin Nov 15, 2024

jrconlin Nov 15, 2024

taddes Nov 19, 2024

jrconlin Nov 19, 2024

pjenvey Nov 20, 2024

taddes Nov 20, 2024 •

edited

Loading

jrconlin Nov 15, 2024

pjenvey Nov 19, 2024 •

edited

Loading

jrconlin Nov 15, 2024

pjenvey Nov 18, 2024

taddes Nov 18, 2024

pjenvey Nov 18, 2024

taddes Nov 19, 2024

pjenvey Nov 19, 2024

taddes Nov 20, 2024

jrconlin Nov 19, 2024

jrconlin Nov 19, 2024

jrconlin Nov 19, 2024

jrconlin Nov 19, 2024

jrconlin Nov 19, 2024

jrconlin Nov 19, 2024

jrconlin Nov 19, 2024

taddes commented Nov 23, 2024

		app_id: "syncstorage".to_string(),
		app_display_version: "1.0.0".to_string(),

feat: glean metrics logic #1626

Are you sure you want to change the base?

feat: glean metrics logic #1626

Conversation

taddes commented Nov 4, 2024 • edited Loading

Description

Testing

Issue(s)

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

taddes Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjenvey Nov 19, 2024 • 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

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

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

taddes commented Nov 23, 2024

taddes commented Nov 4, 2024 •

edited

Loading

taddes Nov 20, 2024 •

edited

Loading

pjenvey Nov 19, 2024 •

edited

Loading