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 metadata spec with host domain name information #795

Closed
wants to merge 6 commits into from

Conversation

stevejgordon
Copy link
Contributor

@stevejgordon stevejgordon commented May 5, 2023

Per #793 - "ECS changed the definition of host.name and encourages to use the fully qualified domain name for the host name". This PR updates the metadata spec with a proposed section detailing how clients should send the domain information required by APM server.

  • 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? The domain name of the host does not hold personal information.
    • 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
    To auto-merge the PR, add /schedule YYYY-MM-DD to the PR description.
  • Create implementation issues through the meta issue template (this will automate issue creation for individual agents)
  • If this spec adds a new dynamic config option, add it to central config.

Closes #794

@stevejgordon
Copy link
Contributor Author

The piece I'm less certain of is the final paragraph. Arguably the configured_hostname name field implies it should be ONLY the hostname but in reality, it could accept any name the user decides to use. Ideally, this would be named configured_host_name to better identify it as the user-defined name of the host.

specs/agents/metadata.md Outdated Show resolved Hide resolved
specs/agents/metadata.md Outdated Show resolved Hide resolved
@stevejgordon stevejgordon marked this pull request as ready for review May 11, 2023 19:38
@stevejgordon stevejgordon requested review from a team as code owners May 11, 2023 19:38
Copy link
Member

@SylvainJuge SylvainJuge left a comment

Choose a reason for hiding this comment

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

Do you think it would be relevant to add in this spec the OS-specific ways to capture the hostname / FQDN ?

I know most of the time we will rely on platform-specific APIs, but it would definitely help implementation to have at least one or more strategy described per OS to:

  • get a consistent reference value that we can use for testing (manual or automated)
  • provide a fallback option in case an equivalent platform API is not available.

specs/agents/metadata.md Outdated Show resolved Hide resolved
Co-authored-by: SylvainJuge <763082+SylvainJuge@users.noreply.github.com>
@stevejgordon
Copy link
Contributor Author

Do you think it would be relevant to add in this spec the OS-specific ways to capture the hostname / FQDN?

Yes, that's a good suggestion. I captured an example of what .NET does in Slack, but I will add and expand on that here to describe possible options for accessing this information.

@stevejgordon
Copy link
Contributor Author

@SylvainJuge Added reference implementation details as suggested.

Copy link
Member

@AlexanderWert AlexanderWert left a comment

Choose a reason for hiding this comment

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

Discussed this with the server team.
We might simplify by requiring all APM agents just to report the fqdn for the hostname instead.

@AlexanderWert
Copy link
Member

AlexanderWert commented Jun 13, 2023

IMPORTANT UPDATE after more discussions and clarifications: We keep it simple for APM agents. This change only requires the APM agents to always report the FQDN (instead of the simple) for the detected_hostname field.

Closing this PR as it is not relevant anymore in that form.

@SylvainJuge SylvainJuge mentioned this pull request Jun 14, 2023
12 tasks
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 793] Spec: Report FQDN for detected_hostname
5 participants