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

RUMM-638 Global RUM attributes #232

Merged
merged 1 commit into from
Aug 31, 2020

Conversation

buranmert
Copy link
Contributor

What and why?

Users may want to use common attributes in their RUM events
We should provide them a convenient way to set those attributes

How?

Now users can add, update or remove global attributes in their RUMMonitor instances

rumMonitor.setAttribute(forKey: "some key", value: "some value") // adds an attribute
rumMonitor.setAttribute(forKey: "some key", value: "some other value") // updates the value
rumMonitor.setAttribute(forKey: "some key", value: nil) // removes the key
let globalAttrs = rumMonitor.attributes // returns current global attributes to be added to events

Note: attributes is thread-safe

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference

@buranmert buranmert added this to the rum milestone Aug 26, 2020
@buranmert buranmert self-assigned this Aug 26, 2020
@buranmert buranmert marked this pull request as ready for review August 26, 2020 14:44
@buranmert buranmert requested a review from a team as a code owner August 26, 2020 14:44
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Few comments added 👌. Also, before we merge it, let's discuss with the team the precise product requirements for global attributes processing.

@buranmert buranmert force-pushed the buranmert/RUMM-638-global-rum-attributes branch from df30050 to 6c0a492 Compare August 31, 2020 09:43
@buranmert buranmert requested a review from ncreated August 31, 2020 09:44
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Just fix the variable name thing, then we can merge 👌

// MARK: - Private

private func aggregate(_ localAttributes: [AttributeKey: AttributeValue]?) -> [AttributeKey: AttributeValue] {
var mergedAttributes = queue.sync { return self.RUMAttributes }
Copy link
Member

Choose a reason for hiding this comment

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

Ref, I created RUMM-691 Add RUMMonitor benchmarks so we can test if there's no performance drop due to this extra thread switch for every public API call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think the cost should be negligible but if we have concerns, we can do that:

protocol RUMCommand {
  var attributes { get set }
  ...
}
class RUMMonitor {
  func process(cmd) {
    queue.async {
      cmd.attributes.merge(globalAttrs)
      appScope.process(cmd)
    }
  }

then we won't need queue.sync to get global attrs
let me know if you need this

@buranmert buranmert force-pushed the buranmert/RUMM-638-global-rum-attributes branch from 6c0a492 to a5480b1 Compare August 31, 2020 14:44
@buranmert buranmert merged commit 8088345 into rum Aug 31, 2020
@buranmert buranmert deleted the buranmert/RUMM-638-global-rum-attributes branch August 31, 2020 15:15
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