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

Add user attribution API #1954

Merged
merged 6 commits into from
May 4, 2022
Merged

Add user attribution API #1954

merged 6 commits into from
May 4, 2022

Conversation

lloeki
Copy link
Member

@lloeki lloeki commented Mar 31, 2022

User attribution enables the user to conveniently tag a trace as being tied to a specific user.

Behind the scenes this leverages setting a tag at the trace-level. Since trace transport does not have a concept of trace, tags are set on the topmost span.

When partial flushing is involved, the topmost span will be sent upon last flush, therefore tags will only appear in the UI when that final segment is processed.

@lloeki lloeki requested a review from a team March 31, 2022 11:22
delner
delner previously requested changes Mar 31, 2022
Copy link
Contributor

@delner delner left a comment

Choose a reason for hiding this comment

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

Adding set_tag should be okay, but I don't think the implementation of this is quite right.

  • set_user should not be a facet of the Trace API as its not a part of the trace specification. I think it would be more appropriate if this function were made a part of the AppSec module wrapping the TraceOperation#set_tag behavior.
  • set_tag shouldn't set tags directly on the root span; not at this level anyways. "Root span" is is a concrete implementation we're trying to abstract away, and TraceOperation is agnostic to how traces are serialized. I think it would be more appropriate if TraceOperation accumulated tags for the trace separate from any span, serialized them into TraceSegment on #flush!, then let the TraceFormatter apply tags as appropriate to the spans being flushed (e.g. root span or local root span).

@lloeki lloeki force-pushed the add-user-tracking-api branch from 9b4a3e7 to 48d12fa Compare April 4, 2022 10:29
@lloeki
Copy link
Member Author

lloeki commented Apr 4, 2022

As discussed, PR split in two parts: user attribution and trace-level tags, this PR being based on #1959.

@lloeki lloeki force-pushed the add-user-tracking-api branch 3 times, most recently from d8365d8 to a23820e Compare April 6, 2022 11:35
@lloeki lloeki force-pushed the add-user-tracking-api branch 2 times, most recently from c534781 to 6566bf9 Compare April 14, 2022 18:40
@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2022

Codecov Report

Merging #1954 (a05b16a) into master (a89d2b2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master    #1954    +/-   ##
========================================
  Coverage   97.74%   97.74%            
========================================
  Files        1004     1007     +3     
  Lines       50738    50902   +164     
========================================
+ Hits        49592    49754   +162     
- Misses       1146     1148     +2     
Impacted Files Coverage Δ
lib/datadog/core.rb 87.50% <100.00%> (+1.78%) ⬆️
lib/datadog/kit.rb 100.00% <100.00%> (ø)
lib/datadog/kit/identity.rb 100.00% <100.00%> (ø)
lib/ddtrace.rb 100.00% <100.00%> (ø)
spec/datadog/kit/identity_spec.rb 100.00% <100.00%> (ø)
lib/datadog/core/diagnostics/environment_logger.rb 98.41% <0.00%> (-1.59%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@lloeki lloeki requested review from delner, ivoanjo and marcotc April 19, 2022 10:47
@lloeki lloeki added this to the 1.0.0.beta3 milestone Apr 22, 2022
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

This seems reasonable but I think @delner and @marcotc have more insight in this area than me ;)

lib/datadog/kit.rb Outdated Show resolved Hide resolved
@lloeki lloeki force-pushed the add-user-tracking-api branch from a68843b to 68a9053 Compare April 27, 2022 13:27
lib/datadog/kit.rb Outdated Show resolved Hide resolved
@lloeki lloeki force-pushed the add-user-tracking-api branch 3 times, most recently from d2b27d0 to c2136b2 Compare April 29, 2022 09:43
@lloeki
Copy link
Member Author

lloeki commented Apr 29, 2022

Huh, CI flakily failed again: JRuby was stuck for 40+min after success and 2.4 (this time) failed after success with:

Too long with no output (exceeded 10m0s): context deadline exceeded

Canceled and restarted.

@lloeki lloeki dismissed delner’s stale review April 29, 2022 10:40

Changes addressed: TraceOperation#set_tag has been implemented separately. set_user is not on TraceOperation anymore.

@lloeki lloeki force-pushed the add-user-tracking-api branch from c2136b2 to f12229e Compare April 29, 2022 10:47
lloeki added 4 commits April 30, 2022 00:03
User attribution enables the user to conveniently tag a trace as being
tied to a specific user.

Behind the scenes this leverages setting a tag at the trace-level.
@lloeki lloeki force-pushed the add-user-tracking-api branch from f12229e to a05b16a Compare April 29, 2022 22:03
@lloeki lloeki removed this from the 1.0.0 milestone Apr 29, 2022
@lloeki lloeki merged commit dfdb18b into master May 4, 2022
@lloeki lloeki deleted the add-user-tracking-api branch May 4, 2022 09:57
@github-actions github-actions bot added this to the 1.1.0 milestone May 4, 2022
@lloeki
Copy link
Member Author

lloeki commented May 4, 2022

Duh, my Datadog PGP key expired yesterday /facepalms/ (actually they all expired, my other ones since March /double-facepalms/)

@lloeki
Copy link
Member Author

lloeki commented May 5, 2022

Added docs in DataDog/documentation#13941

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.

5 participants