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

Set client.ip for iOS agent #4975

Merged
merged 2 commits into from
Mar 23, 2021
Merged

Set client.ip for iOS agent #4975

merged 2 commits into from
Mar 23, 2021

Conversation

axw
Copy link
Member

@axw axw commented Mar 22, 2021

Motivation/summary

Introduce a new "ClientMetadata" gRPC interceptor which records information about the gRPC client. The logic was extracted from the logging interceptor, which now relies on the client metadata interceptor being installed before it.

Also, move the gRPC auth interceptor after logging and metrics so auth failures are logged and included in metrics.

Checklist

How to test these changes

  1. Instrument an iOS application with https://github.com/elastic/apm-agent-ios, pointed at APM Server.
  2. Check that event documents have a client.ip field which matches the IP of the iOS application's device.

Related issues

Closes #4935

@apmmachine
Copy link
Contributor

apmmachine commented Mar 22, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #4975 updated

  • Start Time: 2021-03-23T03:08:29.955+0000

  • Duration: 43 min 1 sec

  • Commit: 6b8e99d

Test stats 🧪

Test Results
Failed 0
Passed 6258
Skipped 120
Total 6378

Trends 🧪

Image of Build Times

Image of Tests

Introduce a new "ClientMetadata" gRPC interceptor,
which records information about the gRPC client.
The logic was extracted from the logging interceptor,
which now relies on the client metadata interceptor
being installed before it.

Also, move the gRPC auth interceptor after logging
and metrics so auth failures are logged and included
in metrics.
@axw axw marked this pull request as ready for review March 22, 2021 05:11
@axw axw requested a review from a team March 22, 2021 05:11

// ContextWithClientMetadata returns a copy of ctx with values.
func ContextWithClientMetadata(ctx context.Context, values ClientMetadataValues) context.Context {
return context.WithValue(ctx, clientMetadataKey{}, values)
Copy link
Contributor

Choose a reason for hiding this comment

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

for my own education: using clientMetadataKey{} provides us with an identifier to extract the values, but it also doesn't allocate because it's an empty struct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Using an unexported struct type as the key is recommended by the Go docs (https://golang.org/pkg/context/):

// A key identifies a specific value in a Context. Functions that wish
// to store values in Context typically allocate a key in a global
// variable then use that key as the argument to context.WithValue and
// Context.Value. A key can be any type that supports equality;
// packages should define keys as an unexported type to avoid
// collisions.

// ClientMetadataFromContext returns client metadata extracted by the ClientMetadata interceptor.
func ClientMetadataFromContext(ctx context.Context) (ClientMetadataValues, bool) {
values, ok := ctx.Value(clientMetadataKey{}).(ClientMetadataValues)
return values, ok
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason you're assigning to variables instead of return ctx.Value(...)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because that doesn't compile :)
return ctx.Value(...) would compile, but the type assertion requires assignment to intermediate variables.

@stuartnelson3
Copy link
Contributor

Instrument an iOS application with https://github.com/elastic/apm-agent-ios, pointed at APM Server.

is this something that's easy to do? or is it requiring running some app in xcode etc etc

@axw
Copy link
Member Author

axw commented Mar 23, 2021

Instrument an iOS application with https://github.com/elastic/apm-agent-ios, pointed at APM Server.

is this something that's easy to do? or is it requiring running some app in xcode etc etc

I understand that it currently requires us to use a mac, I suppose running xcode. This will probably fall on @bryce-b to test.

@codecov-io
Copy link

Codecov Report

Merging #4975 (6b8e99d) into master (f8471bf) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4975      +/-   ##
==========================================
+ Coverage   77.21%   77.27%   +0.06%     
==========================================
  Files         175      177       +2     
  Lines       10413    10442      +29     
==========================================
+ Hits         8040     8069      +29     
  Misses       2373     2373              
Impacted Files Coverage Δ
model/modelprocessor/metadata.go 9.09% <ø> (ø)
beater/interceptors/logging.go 100.00% <100.00%> (ø)
beater/interceptors/metadata.go 100.00% <100.00%> (ø)
beater/otlp/clientmetadata.go 100.00% <100.00%> (ø)
beater/server.go 69.86% <100.00%> (+2.21%) ⬆️
model/modelprocessor/environment.go 100.00% <100.00%> (ø)
model/modelprocessor/hostname.go 100.00% <100.00%> (ø)
model/modelprocessor/nodename.go 100.00% <100.00%> (ø)
...ack/apm-server/aggregation/txmetrics/aggregator.go 93.24% <0.00%> (ø)
... and 1 more

@axw axw merged commit 3f4379a into elastic:master Mar 23, 2021
@axw axw deleted the otel-client-ip branch March 23, 2021 03:58
axw added a commit to axw/apm-server that referenced this pull request Mar 29, 2021
Introduce a new "ClientMetadata" gRPC interceptor,
which records information about the gRPC client.
The logic was extracted from the logging interceptor,
which now relies on the client metadata interceptor
being installed before it.

Also, move the gRPC auth interceptor after logging
and metrics so auth failures are logged and included
in metrics.
# Conflicts:
#	changelogs/head.asciidoc
axw added a commit that referenced this pull request Mar 29, 2021
Introduce a new "ClientMetadata" gRPC interceptor,
which records information about the gRPC client.
The logic was extracted from the logging interceptor,
which now relies on the client metadata interceptor
being installed before it.

Also, move the gRPC auth interceptor after logging
and metrics so auth failures are logged and included
in metrics.
# Conflicts:
#	changelogs/head.asciidoc
mergify bot pushed a commit that referenced this pull request Apr 27, 2021
Introduce a new "ClientMetadata" gRPC interceptor,
which records information about the gRPC client.
The logic was extracted from the logging interceptor,
which now relies on the client metadata interceptor
being installed before it.

Also, move the gRPC auth interceptor after logging
and metrics so auth failures are logged and included
in metrics.

(cherry picked from commit 3f4379a)

# Conflicts:
#	beater/server.go
#	changelogs/head.asciidoc
#	systemtest/logging_test.go
#	systemtest/otlp_test.go
@simitt
Copy link
Contributor

simitt commented May 12, 2021

@bryce-b tested this with an instrumented iOS app. #5247 was created as a follow up task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply geo-ip data to mobile trace data
6 participants