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

Update network connection fields #5671

Merged
merged 7 commits into from
Aug 4, 2021
Merged

Conversation

axw
Copy link
Member

@axw axw commented Jul 12, 2021

Motivation/summary

The OTel semantic conventions for network connection type have evolved: net.host.connection_type is now net.host.connection.type and net.host.connection.subtype. See open-telemetry/opentelemetry-specification#1759. This PR extends #5436 to adapt to these changes.

Support Elastic APM agents sending "network.connection.type" ("n.c.t" in RUMv3) in metadata, which will be added to all events in the stream. This is currently intended for RUM. In the future we may want to support setting this at the even level too, to cater for operation-specific network information.

Finally, the Elasticsearch field names have been changed to network.connection.type and network.connection.subtype.

Checklist

How to test these changes

  1. Run the iOS agent
  2. Check that the network.connection.type and network.connection.subtype fields are recorded

Related issues

#5203

@apmmachine
Copy link
Contributor

apmmachine commented Jul 12, 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 preview

Expand to view the summary

Build stats

  • Start Time: 2021-08-04T07:48:52.157+0000

  • Duration: 41 min 37 sec

  • Commit: 94fe9fd

Test stats 🧪

Test Results
Failed 0
Passed 5934
Skipped 14
Total 5948

Trends 🧪

Image of Build Times

Image of Tests

@axw axw force-pushed the network-connection-subtype branch from f1fa462 to 4ab0d69 Compare July 12, 2021 05:49
@axw axw marked this pull request as ready for review July 12, 2021 06:26
@axw axw requested a review from a team July 12, 2021 06:26
Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

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

What is the urgency of this PR being backported to 7.14? Does the iOS agent not work with the current fields, or would the network.connection* fields just be ignored?

model/error/_meta/fields.yml Outdated Show resolved Hide resolved
@bryce-b
Copy link
Contributor

bryce-b commented Jul 12, 2021

@simitt I mentioned to Andrew in slack: we don't need to backport this to 7.14; I've been holding off on updating the agent with these spec changes due to the Otel spec change not being merged yet.

@mergify
Copy link
Contributor

mergify bot commented Jul 14, 2021

This pull request is now in conflicts. Could you fix it @axw? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b network-connection-subtype upstream/network-connection-subtype
git merge upstream/master
git push upstream network-connection-subtype

@axw axw marked this pull request as draft August 2, 2021 01:53
axw added 3 commits August 3, 2021 12:10
Change network.connection_type to network.connection.type,
and add network.connection.subtype.

Rename model.Carrier to model.NetworkCarrier to clarify its
position within the document hierarchy.
Support agents sending "network.connection.type"
("n.c.t" in RUMv3) in metadata, which will be added
to all events in the stream. This is currently
intended for RUM. In the future we may want to
support setting this at the even level too, to
cater for operation-specific network information.
@axw axw changed the title Update OTel network connection type conventions Update network connection fields Aug 3, 2021
@axw axw force-pushed the network-connection-subtype branch from 4ab0d69 to 06bedaf Compare August 3, 2021 04:24
@axw axw requested a review from a team August 3, 2021 05:25
@axw axw marked this pull request as ready for review August 3, 2021 05:26
@axw axw enabled auto-merge (squash) August 4, 2021 02:30
@mergify
Copy link
Contributor

mergify bot commented Aug 4, 2021

This pull request is now in conflicts. Could you fix it @axw? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b network-connection-subtype upstream/network-connection-subtype
git merge upstream/master
git push upstream network-connection-subtype

@axw axw merged commit d1ebfd4 into elastic:master Aug 4, 2021
mergify bot pushed a commit that referenced this pull request Aug 4, 2021
* Update OTel network connection type conventions

* Add network.connection.subtype field

Change network.connection_type to network.connection.type,
and add network.connection.subtype.

Rename model.Carrier to model.NetworkCarrier to clarify its
position within the document hierarchy.

* model/modeldecoder: add "network.connection.type"

Support agents sending "network.connection.type"
("n.c.t" in RUMv3) in metadata, which will be added
to all events in the stream. This is currently
intended for RUM. In the future we may want to
support setting this at the even level too, to
cater for operation-specific network information.

* Update changelog

(cherry picked from commit d1ebfd4)

# Conflicts:
#	changelogs/head.asciidoc
#	include/fields.go
axw added a commit that referenced this pull request Aug 5, 2021
* Update network connection fields (#5671)

* Update OTel network connection type conventions

* Add network.connection.subtype field

Change network.connection_type to network.connection.type,
and add network.connection.subtype.

Rename model.Carrier to model.NetworkCarrier to clarify its
position within the document hierarchy.

* model/modeldecoder: add "network.connection.type"

Support agents sending "network.connection.type"
("n.c.t" in RUMv3) in metadata, which will be added
to all events in the stream. This is currently
intended for RUM. In the future we may want to
support setting this at the even level too, to
cater for operation-specific network information.

* Update changelog

(cherry picked from commit d1ebfd4)

# Conflicts:
#	changelogs/head.asciidoc
#	include/fields.go

* Fix merge conflicts

Co-authored-by: Andrew Wilkins <[email protected]>
@axw axw deleted the network-connection-subtype branch August 19, 2021 00:49
@simitt simitt self-assigned this Aug 31, 2021
@simitt
Copy link
Contributor

simitt commented Aug 31, 2021

Cannot yet be tested as the latest version of the iOS agent does not contain the change; it still sends net.host.connection_type instead of net.host.connection.type, see https://github.com/elastic/apm-agent-ios/blob/main/Sources/Instrumentation/NetworkInfo/NetworkStatusInjector.swift#L28.

@bryce-b any update on when this will be changed in the agent?

@bryce-b
Copy link
Contributor

bryce-b commented Sep 7, 2021

@Silvia These changes will be in this week.

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.

6 participants