Skip to content

Conversation

@z1c0
Copy link
Contributor

@z1c0 z1c0 commented Aug 30, 2022

  • May the instrumentation collect sensitive information, such as secrets or PII (ex. in headers)?
    • Yes
      • Add a section to the spec how agents should apply sanitization (such as sanitize_field_names)
    • No
      • Why?
    • n/a
  • Create PR as draft
  • Approval by at least one other agent
  • Mark as Ready for Review (automatically requests reviews from all agents and PM via CODEOWNERS)
    • Remove PM from reviewers if impact on product is negligible
    • Remove agents from reviewers if the change is not relevant for them
  • Approved by at least 2 agents + PM (if relevant)
  • Merge after 7 days passed without objections
  • Create implementation issues through the meta issue template (this will automate issue creation for individual agents)

@z1c0 z1c0 requested a review from AlexanderWert August 30, 2022 07:57
@z1c0 z1c0 linked an issue Aug 30, 2022 that may be closed by this pull request
@ghost
Copy link

ghost commented Aug 30, 2022

💚 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: 2022-09-19T05:48:00.114+0000

  • Duration: 3 min 8 sec

Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

LGTM!
I think you make this PR ready for review.
Can you provide a couple of examples for /proc/self/cgroup file lines that we can use for unit testing?

@z1c0
Copy link
Contributor Author

z1c0 commented Sep 7, 2022

LGTM! I think you make this PR ready for review.

Thanks, will do!

Can you provide a couple of examples for /proc/self/cgroup file lines that we can use for unit testing?

In apm-agent-dotnet, this unit test was added (id: 03752a671e744971a862edcee6195646-4015103728).

@z1c0 z1c0 marked this pull request as ready for review September 7, 2022 18:52
@z1c0 z1c0 requested review from a team as code owners September 7, 2022 18:52
@felixbarny
Copy link
Member

felixbarny commented Sep 8, 2022

Can you provide a couple of examples for /proc/self/cgroup file lines that we can use for unit testing?

In apm-agent-dotnet, this unit test was added (id: 03752a671e744971a862edcee6195646-4015103728).

I think it makes sense to add a common json test spec in https://github.com/elastic/apm/tree/main/tests/agents/json-specs for this. I don't see this as blocking this PR, though.

@z1c0
Copy link
Contributor Author

z1c0 commented Sep 8, 2022

Can you provide a couple of examples for /proc/self/cgroup file lines that we can use for unit testing?

In apm-agent-dotnet, this unit test was added (id: 03752a671e744971a862edcee6195646-4015103728).

I think it makes sense to add a common json test spec in https://github.com/elastic/apm/tree/main/tests/agents/json-specs for this. I think see this as blocking this PR, though.

Make sense, I wasn't aware this existed though :-)
I'll update the test spec and (add a test in elastic/apm-agent-dotnet#1805)

@z1c0 z1c0 merged commit 74efc60 into main Sep 19, 2022
@z1c0 z1c0 deleted the 677-meta-676-spec-update-metadata-spec-regarding-ecsfargate-pod-uids branch September 19, 2022 11:53
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.

[META 676] Spec: Update metadata spec regarding ECS/Fargate pod UIDs

8 participants