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

fix: adjusting container instrumentation infrastructure monitoring to highlight assignment of container logs #16709

Closed
wants to merge 19 commits into from

Conversation

harrykimpel
Copy link

Please follow conventional commit standards
in your commit messages and pull request title.

Give us some context

  • What problems does this PR solve?
    This PR will highlight some required changes in order to assign container logs to our container entities rather than the host entity

  • Add any context that will help us review your changes such as testing notes,
    links to related docs, screenshots, etc.
    there are some threads in our forum (see here and here)

  • If your issue relates to an existing GitHub issue, please link to it.
    n/a

@CLAassistant
Copy link

CLAassistant commented Mar 28, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link

Hi @harrykimpel 👋

Thanks for your pull request! Your PR is in a queue, and a writer will take a look soon. We generally publish small edits within one business day, and larger edits within three days.

We will automatically generate a preview of your request, and will comment with a link when the preview is ready (usually 10 to 20 minutes). If you add any more commits, you can comment netlify build on this PR to update the preview.

Copy link

netlify bot commented Mar 28, 2024

Deploy Preview for docs-website-netlify ready!

Name Link
🔨 Latest commit 82ca940
🔍 Latest deploy log https://app.netlify.com/sites/docs-website-netlify/deploys/66311e29c2d4ad0008dccc73
😎 Deploy Preview https://deploy-preview-16709--docs-website-netlify.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@harrykimpel harrykimpel changed the title fix: adjusting container instrumentation infrastructure monitoring to highlight fix: adjusting container instrumentation infrastructure monitoring to highlight assignment of container logs Mar 28, 2024
@akristen akristen self-assigned this Mar 28, 2024
@akristen akristen added content requests related to docs site content from_internal Identifies issues/PRs from Relics (except writers) labels Mar 28, 2024
@akristen
Copy link
Contributor

akristen commented Apr 4, 2024

Taking a look at this today!

This commit does not include updating pathing to add log parsing rules in the UI. I am checking if it will build remotely

Our [infrastructure agent](/docs/infrastructure/infrastructure-monitoring/get-started/get-started-infrastructure-monitoring) can gather logs from your containers. When you enable logs from the infrastructure agent, the agent will automatically assign those logs to the hosts running Docker Compose.

## Requirements [#requirements]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do customers need to have instrumented their containers? Should we instead link them to the other containers dock as a prerequisite? I imagine they can't collect logs about containers w/o having instrumentation first.

@harrykimpel

Copy link
Author

@harrykimpel harrykimpel Apr 11, 2024

Choose a reason for hiding this comment

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

@akristen yes, that is correct. I guess it makes sense to link them to the other containers doc.

@akristen akristen closed this Apr 10, 2024
@akristen akristen reopened this Apr 10, 2024
@akristen
Copy link
Contributor

akristen commented Apr 10, 2024

leaving a note for myself to update the URL in i18n (or revert that change) once i get SME approval from Core team

Copy link
Author

@harrykimpel harrykimpel left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@jsubirat jsubirat left a comment

Choose a reason for hiding this comment

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

Hey @harrykimpel, I think there could be a safer (and easier) alternative here, to ensure that logs are cleanly associated with a container and they do not get associated with a host. We could modify the Fluent Bit configuration generated by the Infra-Agent so that it removes the entity.guid.INFRA of the host if it detects that it comes from a Docker container, and then just renames attrs.tag into containerName. I think we could (and should) implement this as code, and do it out-of-the-box in the Infra-Agent. Please let me discuss this internally and will get back to you. For the time being, I'd like to ask this to wait a little bit before getting merged.

Comment on lines 36 to 43
<img
title="Add the containers path to your snippet"
alt="A screenshot that shows where to add this snippet in context of your yaml file"
src={infrastructureAddContainersLogPath}
/>

* You may find this file at this path: `/etc/newrelic-infra/logging.d/logging.yml`.
* This section directs the agent to collect logs from your containers
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably don't need to add this. Please note that the Log Forwarder integration picks any YAML file under /etc/newrelic-infra/logging.d/*.yml and translates them to Fluent Bit configuration. Contrarily to the public belief, they don't need to be placed under a single /etc/newrelic-infra/logging.d/logging.yml file. I believe this missconception was caused by Virtuoso, because that is the file it is using.

In this case, I think it makes sense to just use a separate file to include the Docker container logs config. It results in simpler installation instructions as well, IMHO, as we don't need to tell the user "edit the file in this particular way" (error prone, specially when editing YAML files), but just "create a file with these exact contents".


</figcaption>

5. Generate some data, wait a few minutes, and you'll see container logs assigned to your containers running on your host, rather than assigned to the host itself.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused here. The Infra-Agent log forwarder normally gets the entity.guid belonging to the host running these containers, so the logs get associated to the host. Are you doing something differently in this case, when running Docker containers? My understanding is that by having containerName in the logs, you are somehow leveraging some Entity Synthesis rule to associate the log to a particular container. And I assume this is happening because the Infra-Agent actually sends entity.guid.INFRA, and not entity.guid. So by the time entity synthesis runs, entity.guid is still not populated.

I'm afraid that this solution is making some assumptions in the order in which the backend will perform the actions (that is, first it will execute parsing, then entity synthesis, and later on entity.guid consolidation), so I'd probably advise against this. I'd not like to have this coupling in the pipeline, as it may block us from being able to evolve it and modify the step order if we ever need to.

PS: have you verified that if logs get associated to the container they don't get associated to the host as well?

@akristen
Copy link
Contributor

I'm adding this doc to sprint starting 29 APR

@akristen
Copy link
Contributor

Closing this PR until @jsubirat 's team notifies us with a better solution to complete this action.

@akristen akristen closed this May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content requests related to docs site content from_internal Identifies issues/PRs from Relics (except writers)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants