Skip to content

Conversation

@dashed
Copy link
Member

@dashed dashed commented May 5, 2021

Begin emitting breakdowns for all transactions. The performance-ops-breakdown will instead be used to gate the UI product features for displaying these breakdowns.

Related PR: getsentry/relay#993

@dashed dashed requested a review from a team May 5, 2021 16:38
@dashed dashed self-assigned this May 5, 2021
@dashed dashed requested a review from a team May 5, 2021 16:38
Copy link
Member

@k-fish k-fish left a comment

Choose a reason for hiding this comment

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

👍

@dashed
Copy link
Member Author

dashed commented May 5, 2021

@untitaker It seems like we have these assertions in the Relay project options tests (and in other places):

# Sweeping assertion that we do not have any snake_case in that config.
# Might need refining.
assert not {x for x in _get_all_keys(result) if "-" in x or "_" in x}

And enabling the breakdowns project options is failing these assertions.

The project options are as follows:

{
   "breakdowns":{
      "span_ops":{
         "matches":[
            "http",
            "db",
            "browser",
            "resource"
         ],
         "type":"span_operations"
      }
   }
}

Can you advise if the dictionary keys should not contain hyphens or underscores?

@untitaker
Copy link
Member

untitaker commented May 6, 2021

@dashed we used to have a bug where we accidentally returned snakecase for all options in Sentry even though they were encoded as camelCase in Relay. If it is possible I would make every key/literal/constant/symbol in project config camelCase for consistency.

Just took a quick look around the code, so I hope the following makes sense. I would:

  1. change the type value to camelCase: https://github.com/getsentry/relay/blob/e3c064e213281c36bde5d2b6f3032c6d36e22520/relay-general/src/store/normalize/breakdowns.rs#L205
  2. modify the assertion such that span_ops specifically is exempted from those checks, as it seems that this is passed through into the event (I'd say event schema is more important to keep internally consistent)

Copy link
Member

Choose a reason for hiding this comment

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

1: {
"span_ops": {
"type": "span_operations",
"type": "spanOperations",
Copy link
Member Author

Choose a reason for hiding this comment

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

@untitaker I'm going to rename this to camel case for the project options.

@dashed dashed requested a review from untitaker May 13, 2021 23:30
Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

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

Please absolutely make sure to merge the Relay PR and deploy it first.

@dashed
Copy link
Member Author

dashed commented May 17, 2021

/gcbrun

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants