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(dynamic_config): Add config for dynamic metrics extraction #2252

Merged
merged 10 commits into from
Jun 27, 2023

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented Jun 26, 2023

Adds configuration for dynamic metrics extraction to project configuration. This is inactive at this moment, as actual extraction will be implemented in a follow-up.

Ref #2237

@jan-auer jan-auer marked this pull request as ready for review June 27, 2023 08:00
@jan-auer jan-auer requested a review from a team June 27, 2023 08:00
Copy link
Contributor

@olksdr olksdr left a comment

Choose a reason for hiding this comment

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

Left few questions , otherwise LGTM

relay-dynamic-config/src/metrics.rs Outdated Show resolved Hide resolved
Comment on lines +225 to +226
/// defined through `field`. These two options are mutually exclusive, behavior is undefined if both
/// are specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is the false statement here, if both specified we always return field as a source on line 255.
Or will be there more code which will make this behavior undefined ?

Copy link
Member Author

Choose a reason for hiding this comment

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

My intent was to leave behavior undefined on the protocol level (hence the doc comment), but of course we need to do something. At this point, I'd like to leave our options open to throw an error, use both as a fallback, or just have one take precedence over the other. Either way, at this point we do not expect Sentry to emit entries with both set.

Co-authored-by: Oleksandr <1931331+olksdr@users.noreply.github.com>
@jan-auer jan-auer enabled auto-merge (squash) June 27, 2023 08:33
Copy link
Contributor

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

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

LGTM. Mostly comments on docs and a few non-urgent questions.

relay-dynamic-config/src/project.rs Show resolved Hide resolved
relay-dynamic-config/src/metrics.rs Outdated Show resolved Hide resolved
relay-dynamic-config/src/metrics.rs Outdated Show resolved Hide resolved
relay-dynamic-config/src/metrics.rs Show resolved Hide resolved
relay-dynamic-config/src/metrics.rs Outdated Show resolved Hide resolved
relay-dynamic-config/src/metrics.rs Outdated Show resolved Hide resolved
relay-dynamic-config/src/metrics.rs Outdated Show resolved Hide resolved
relay-dynamic-config/src/metrics.rs Outdated Show resolved Hide resolved
relay-dynamic-config/src/metrics.rs Outdated Show resolved Hide resolved
} else if let Some(ref value) = self.value {
TagSource::Literal(value)
} else {
TagSource::Unknown
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about logging an error in this case, at least for the first few iterations?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we make changes during our first iterations, this error would also show up for customers if they run a Relay with this intermediate version. Since this is future-proofing, it's better if we keep this silent. There can be other tests that ensure our config roundtrips in Sentry.

/// Mapping between extracted metrics and additional tags to extract.
#[derive(Clone, Debug, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct TagMapping {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like TagMapping, but don't really have better alternatives. What do you think about SharedTags?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could do SharedTagMapping, would that be better?

Note: I might merge this PR sooner, but we can follow up with some cleanup.

@jan-auer jan-auer merged commit f8bf782 into master Jun 27, 2023
@jan-auer jan-auer deleted the feat/metrics-extraction-config branch June 27, 2023 11:29
jan-auer added a commit that referenced this pull request Jun 27, 2023
* master:
  feat(dynamic_config): Add config for dynamic metrics extraction (#2252)
jan-auer added a commit that referenced this pull request Jun 28, 2023
…2257)

Adds an initial version of generic metrics extraction based on the
previously added schema (#2252). It is limited to transaction events and
evaluates conditions with the fields supported by `FieldValueProvider`.

Since generic metric extraction can operate on all item types, the a new
top-level processing function is added that runs before the more
specific transaction metric extraction. In the future, we will convert
conditional tagging logic and even transaction metric extraction
configuration to the new format when the project config is read, so that
the old functions can be removed.

Since the new function supports only transactions, it reuses the
existing `transaction_metrics_extracted` flag from the processing state.
This is subject to change in a follow-up.
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.

3 participants