[processor/attributes] Support attributes set by server authenticators #9420

svrakitin · 2022-04-24T12:51:58Z

Description:

Support looking up attributes from client.Info.Auth which is set by extensions implementing configauth.ServerAuthenticator. This way we can attach authentication of the exporter (e.g. OIDC subject) to the telemetry.

User can refer to these attributes by prefixing keys with auth. when using from_context:

attributes/subject:
  actions:
  - key: enduser.id
    from_context: auth.subject
    action: insert

For consistency I added metadata. prefix to refer to actual metadata/header keys. This prefix is optional for backwards compatibility.

Testing:

I added tests to attraction package which has actual extraction logic.

Closes #9241

internal/coreinternal/attraction/attraction.go

jpkrohling

LGTM, just the documentation could be a bit better. Thanks for this PR!

CHANGELOG.md

internal/coreinternal/attraction/attraction.go

processor/attributesprocessor/README.md

processor/attributesprocessor/testdata/config.yaml

svrakitin · 2022-04-26T18:31:57Z

@jpkrohling I added logger to the AttrProc and removed its arg from the Process method. Now logger is injected from the processor.

Thanks for suggestions to the docs.

I just noticed there is a relevant issue, thanks for linking it to the PR.

internal/coreinternal/attraction/attraction.go

jpkrohling · 2022-04-26T19:48:30Z

internal/coreinternal/attraction/attraction_test.go

@@ -86,7 +86,7 @@ func TestAttributes_InsertValue(t *testing.T) {
 		},
 	}

-	ap, err := NewAttrProc(cfg)
+	ap, err := NewAttrProc(cfg, nil)


I won't block the PR for this unless @pmm-sumo agrees with me, but I would prefer the test to be passing zap.NewNop() instead of nil. Remember that your test code shows the good practice for consumers of the main code, and by passing nil, you are sending the message that nil is OK, when it really isn't.

Sure, I don't have a preference here. Gonna update.

pmm-sumo · 2022-04-26T20:13:19Z

internal/coreinternal/attraction/attraction.go

-func NewAttrProc(settings *Settings) (*AttrProc, error) {
-	var attributeActions []attributeAction
+func NewAttrProc(settings *Settings, logger *zap.Logger) (*AttrProc, error) {
+	if logger == nil {


I think now we might assume that logger is never nil?

Yeah, it was never nil and that's why passing nil previously worked. I don't want this to fail if nil is passed. :)

It's good to check for nil, but not sure about the outcome: you do want to have access to a logger, it's not an optional parameter, so, you should return an error if it's nil.

I agree, this seems to be unrelated, consider to split a separate PR.

svrakitin · 2022-04-26T20:16:03Z

There is a race detected by tests which seems to be related to open-telemetry/opentelemetry-collector#4939. Not sure.

Asked about this here: https://cloud-native.slack.com/archives/C01N6P7KR6W/p1651007898391749

https://github.com/open-telemetry/opentelemetry-collector-contrib/runs/6182541324?check_suite_focus=true#step:8:18894

rkukura

This all looks good to me - very similar to what I planned to implement when filing #9241, except that I didn't anticipate the logging changes. Please let me know if I can help get this merged.

svrakitin · 2022-04-27T13:36:51Z

@rkukura I probably just need someone to rerun unit tests to get over the flaky ones. Thanks.

jpkrohling · 2022-04-27T14:14:33Z

Tests restarted

jpkrohling · 2022-04-27T14:15:57Z

internal/coreinternal/attraction/attraction.go

-func NewAttrProc(settings *Settings) (*AttrProc, error) {
-	var attributeActions []attributeAction
+func NewAttrProc(settings *Settings, logger *zap.Logger) (*AttrProc, error) {
+	if logger == nil {


It's good to check for nil, but not sure about the outcome: you do want to have access to a logger, it's not an optional parameter, so, you should return an error if it's nil.

internal/coreinternal/attraction/attraction.go

jpkrohling · 2022-04-29T12:55:31Z

PR rebased

svrakitin · 2022-04-29T12:57:12Z

@jpkrohling Thanks.

svrakitin · 2022-05-04T10:37:08Z

@jpkrohling What is left to get it merged?

pmm-sumo · 2022-05-04T11:13:51Z

Juraci is now on parental leave :) But since we have 3 approvals (including two from approvers/contributors) and passing tests, I'm marking this as ready-to-merge

internal/coreinternal/attraction/attraction.go

bogdandrutu · 2022-05-04T12:33:00Z

internal/coreinternal/attraction/attraction.go

-func NewAttrProc(settings *Settings) (*AttrProc, error) {
-	var attributeActions []attributeAction
+func NewAttrProc(settings *Settings, logger *zap.Logger) (*AttrProc, error) {
+	if logger == nil {


I agree, this seems to be unrelated, consider to split a separate PR.

internal/coreinternal/attraction/attraction.go

bogdandrutu · 2022-05-04T12:36:31Z

@pmm-sumo there are lots of unrelated changes, and some random configurations for logger (why do we need sampling here and not in other places? add debug logs if too many, record metrics, etc.)

svrakitin · 2022-05-04T13:03:27Z

@bogdandrutu There was a suggestion to log invalid atrribute types if they are returned by ServerAuthenticator: #9420 (comment)

I don’t care much about sampling and can drop it. It was taken from the suggestion in one of the comments above.

I will reduce this PR to changes I originally introduced and leave further changes to maintainers who have a better idea than me.

bogdandrutu · 2022-05-04T13:24:09Z

@svrakitin it is not about "better idea than me", it is about keeping the PRs focus on what we want to implement :)

pmm-sumo · 2022-05-04T13:46:22Z

Good point @bogdandrutu, apologies for messing up with tagging and thanks for bringing this up.

On the sampled logger thing, I believe it stems from this comment. I mentioned sampled logging since this was something brought to my attention in the past (don't recall which PR but I can look it up for context).

In principle, I think that user should know that something is wrong and because of that debug logs are not a good fit. I also heard some comments that the collector is sometimes too quiet over what happens in the components and some users anticipate more logging. At the same time, we want to limit the impact of that log if happens frequently, hence (aggressive) sampled logger and warn level.

So we have two paths here:

always use debug level for such kind of logs
use sampled logger and move it to a common package, so other components could use it easily for potentially noisy logs

I was inclined towards the latter (in a separate PR). There are some components doing that already and I was aiming at doing the cleanup after this get merged. Before I open the issue wanted to check what is your preference @bogdandrutu

svrakitin · 2022-05-04T14:30:35Z

I reduced this PR to the initial set of changes. Let's address how we warn users in a separate PR then.

PR changed, re-review needed :)

bogdandrutu

This PR looks much better now, @pmm-sumo please re-approve since you are familiar, and add the label again

open-telemetry#9420) Co-authored-by: Bogdan Drutu <bogdandrutu@gmail.com> Co-authored-by: Alex Boten <aboten@lightstep.com>

svrakitin commented Apr 24, 2022

View reviewed changes

internal/coreinternal/attraction/attraction.go Show resolved Hide resolved

svrakitin marked this pull request as ready for review April 24, 2022 13:07

svrakitin requested a review from a team April 24, 2022 13:07

svrakitin requested a review from pmm-sumo as a code owner April 24, 2022 13:07

github-actions bot assigned bogdandrutu Apr 24, 2022

jpkrohling self-requested a review April 25, 2022 14:57

jpkrohling changed the title ~~[processor/attributes] Support attributes set by server authenticator~~ [processor/attributes] Support attributes set by server authenticators Apr 26, 2022

jpkrohling reviewed Apr 26, 2022

View reviewed changes

svrakitin requested a review from dmitryax as a code owner April 26, 2022 18:29

pmm-sumo reviewed Apr 26, 2022

View reviewed changes

internal/coreinternal/attraction/attraction.go Outdated Show resolved Hide resolved

jpkrohling reviewed Apr 26, 2022

View reviewed changes

pmm-sumo reviewed Apr 26, 2022

View reviewed changes

pmm-sumo previously approved these changes Apr 26, 2022

View reviewed changes

rkukura approved these changes Apr 27, 2022

View reviewed changes

jpkrohling approved these changes Apr 27, 2022

View reviewed changes

jpkrohling force-pushed the attributesprocessor-auth-from-context branch from d9230ae to 42fa321 Compare April 29, 2022 12:55

pmm-sumo added the ready to merge Code review completed; ready to merge by maintainers label May 4, 2022

bogdandrutu reviewed May 4, 2022

View reviewed changes

bogdandrutu removed the ready to merge Code review completed; ready to merge by maintainers label May 4, 2022

[processor/attributes] Support attributes set by server authenticators

a147333

svrakitin force-pushed the attributesprocessor-auth-from-context branch from 42fa321 to a147333 Compare May 4, 2022 14:23

bogdandrutu reviewed May 4, 2022

View reviewed changes

pmm-sumo approved these changes May 4, 2022

View reviewed changes

pmm-sumo added the ready to merge Code review completed; ready to merge by maintainers label May 4, 2022

svrakitin and others added 3 commits May 5, 2022 18:16

Merge branch 'main' into attributesprocessor-auth-from-context

d3a425f

Merge branch 'main' into attributesprocessor-auth-from-context

5d51cad

Merge branch 'main' into attributesprocessor-auth-from-context

7ab4165

bogdandrutu merged commit aca7b2f into open-telemetry:main May 5, 2022

ahayworth mentioned this pull request May 16, 2022

Update to otel-contrib-col v0.51.0 Shopify/opentelemetry-collector-contrib#796

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[processor/attributes] Support attributes set by server authenticators #9420

[processor/attributes] Support attributes set by server authenticators #9420

svrakitin commented Apr 24, 2022 •

edited by jpkrohling

Loading

jpkrohling left a comment

svrakitin commented Apr 26, 2022 •

edited

Loading

jpkrohling Apr 26, 2022

svrakitin Apr 26, 2022

pmm-sumo Apr 26, 2022

svrakitin Apr 26, 2022 •

edited

Loading

jpkrohling Apr 27, 2022

bogdandrutu May 4, 2022

svrakitin commented Apr 26, 2022 •

edited

Loading

rkukura left a comment

svrakitin commented Apr 27, 2022 •

edited

Loading

jpkrohling commented Apr 27, 2022

jpkrohling Apr 27, 2022

jpkrohling commented Apr 29, 2022

svrakitin commented Apr 29, 2022

svrakitin commented May 4, 2022

pmm-sumo commented May 4, 2022

bogdandrutu May 4, 2022

bogdandrutu commented May 4, 2022

svrakitin commented May 4, 2022

bogdandrutu commented May 4, 2022

pmm-sumo commented May 4, 2022

svrakitin commented May 4, 2022

bogdandrutu left a comment

[processor/attributes] Support attributes set by server authenticators #9420

[processor/attributes] Support attributes set by server authenticators #9420

Conversation

svrakitin commented Apr 24, 2022 • edited by jpkrohling Loading

jpkrohling left a comment

Choose a reason for hiding this comment

svrakitin commented Apr 26, 2022 • edited Loading

jpkrohling Apr 26, 2022

Choose a reason for hiding this comment

svrakitin Apr 26, 2022

Choose a reason for hiding this comment

pmm-sumo Apr 26, 2022

Choose a reason for hiding this comment

svrakitin Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

jpkrohling Apr 27, 2022

Choose a reason for hiding this comment

bogdandrutu May 4, 2022

Choose a reason for hiding this comment

svrakitin commented Apr 26, 2022 • edited Loading

rkukura left a comment

Choose a reason for hiding this comment

svrakitin commented Apr 27, 2022 • edited Loading

jpkrohling commented Apr 27, 2022

jpkrohling Apr 27, 2022

Choose a reason for hiding this comment

jpkrohling commented Apr 29, 2022

svrakitin commented Apr 29, 2022

svrakitin commented May 4, 2022

pmm-sumo commented May 4, 2022

bogdandrutu May 4, 2022

Choose a reason for hiding this comment

bogdandrutu commented May 4, 2022

svrakitin commented May 4, 2022

bogdandrutu commented May 4, 2022

pmm-sumo commented May 4, 2022

svrakitin commented May 4, 2022

bogdandrutu left a comment

Choose a reason for hiding this comment

svrakitin commented Apr 24, 2022 •

edited by jpkrohling

Loading

svrakitin commented Apr 26, 2022 •

edited

Loading

svrakitin Apr 26, 2022 •

edited

Loading

svrakitin commented Apr 26, 2022 •

edited

Loading

svrakitin commented Apr 27, 2022 •

edited

Loading