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

Initial commit for setting up a new component: k8slog receiver #24439

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

h0cheung
Copy link
Contributor

Description:

k8slog receiver provides full functionality to collect logs in k8s containers. It is an all-in-one solution, which supports to:

  • collect logs from files inside k8s containers via daemonset
  • support docker and cri-containerd
  • support many docker graph drivers / snapshotters
  • collect logs from stdout of k8s containers via daemonset
  • filter containers by metadata
  • extract metadata of containers
  • collect file logs in k8s containers via sidecar

If we want to collect logs from k8s, we can use this component.

Link to tracking Issue:

#23339

Testing:

Tests for unmarshaling configuration have been added.

Documentation:

Docs for configurations and overall design has been added.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Aug 8, 2023
@h0cheung
Copy link
Contributor Author

h0cheung commented Aug 8, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

remove stale

)

// Config is the configuration of a k8s input operator
type Config struct {
Copy link
Member

Choose a reason for hiding this comment

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

Are all of the configuration options required for the initial implementation? Can you remove them for now and add them along with the functionality using them going forward?

Copy link
Member

Choose a reason for hiding this comment

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

I agree, starting small will make things easier to review and get started and we can always add them in the future.

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

@h0cheung I am very excited about this component, sorry I didn't see your proposal sooner. @dmitryax thanks for pointing it out.


// Expr represents the rules to filter containers based on a custom expression.
// TODO: define the values passed to expr.
Expr string `mapstructure:"expr"`
Copy link
Member

Choose a reason for hiding this comment

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

Lets not introduce expr filtering to start. The other filter options you've provided are quite extensive. For expression language filtering I'd prefer we use OTTL.

receiver/k8slogreceiver/config.go Outdated Show resolved Hide resolved
stability:
development: [logs]
codeowners:
active: [h0cheung]
Copy link
Member

Choose a reason for hiding this comment

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

You can add me here as well

Copy link
Contributor Author

@h0cheung h0cheung Aug 20, 2023

Choose a reason for hiding this comment

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

OK, I will do it soon.

)

// Config is the configuration of a k8s input operator
type Config struct {
Copy link
Member

Choose a reason for hiding this comment

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

I agree, starting small will make things easier to review and get started and we can always add them in the future.

receiver/k8slogreceiver/config.go Show resolved Hide resolved
@h0cheung
Copy link
Contributor Author

h0cheung commented Sep 4, 2023

@TylerHelmuth @dmitryax I've simplified the configs and added Tyler Helmuth to the code owner. Could you please have a look at this?

Comment on lines 44 to 48
// Env represents the rules to extract from container environment variables.
Env []FieldExtractConfig `mapstructure:"env"`

// OtelEnv represents the rules to extract from environment variables of otel itself.
OtelEnv []FieldExtractConfig `mapstructure:"otel_env"`
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should allow extracting from env variables, that feels like a security risk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OtelEnv has been removed.
However, Env is needed in some cases. For example, if there are some environment variables in the docker image, there is no other way to get it.
Is this really risky? resourcedetection supports it, and this component gets k8s node name from environment variables, too.

Copy link
Member

Choose a reason for hiding this comment

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

Using an env var to filter feels ok. But this section is for taking fields from the pod and adding them as resource attributes to the log right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Getting attributes from environment variables is useful, and many components support it such as resoucedetection

Are there any reports of security risks associated with reading environment variables as strings and adding them to data?

Copy link
Member

Choose a reason for hiding this comment

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

The risk is exposing apikey/password information, but that risk is lowered if the config only allows static strings. If we do allow this I think we can do all env vars with 1 config option, no need to split out OTel env vars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default, the env config is empty, and no variable will be exposed. When a user set this, he should know what he is doing.

It seems that otel_env is necessary, so I've removed it in the latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

@dmitryax do you have an opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any updates for this?

Copy link
Member

Choose a reason for hiding this comment

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

Lets move forward with this allowed as we've discussed.

Copy link
Member

Choose a reason for hiding this comment

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

I also find it useful to have the option to collect ENV_VAR information from the Pods.
That would give us important information about OTEL specific info like OTEL_SERVICE_NAME.
Having service.name populated in log records collected by the k8slog receiver is quite important since it will allow us to correlate logs with traces using the service.name value.

receiver/k8slogreceiver/config.go Outdated Show resolved Hide resolved
@h0cheung h0cheung force-pushed the feat/k8slog-setup branch 3 times, most recently from 132b260 to 8256f32 Compare September 8, 2023 12:35
@github-actions github-actions bot added the Stale label Jun 27, 2024
@h0cheung
Copy link
Contributor Author

h0cheung commented Jul 8, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

Remove stale

@atoulme atoulme added never stale Issues marked with this label will be never staled and automatically removed and removed Stale labels Jul 8, 2024
@h0cheung
Copy link
Contributor Author

@ChrsMark please take a look at the failing CLA

Can we just temporarily drop his changes to get this merged?

@ChrsMark
Copy link
Member

@h0cheung I have fixed the CLA issues on my side. At least I haven't got any errors the past 2 weeks on PRs/commits.
Can you try to rebase the PR?

In any case, I think there are still major open questions on this PR.

@mx-psi
Copy link
Member

mx-psi commented Jul 22, 2024

/easycla

@h0cheung h0cheung force-pushed the feat/k8slog-setup branch 5 times, most recently from b37ab49 to 1c6b049 Compare July 26, 2024 06:49
@paramsdas-generali
Copy link

@atoulme Are there any plans to integrate the k8slog receiver into the splunk-otel-collector after it becomes available through this merge request?

@h0cheung h0cheung force-pushed the feat/k8slog-setup branch 3 times, most recently from 09fca1f to cb65319 Compare September 19, 2024 12:05
@h0cheung h0cheung requested a review from a team as a code owner September 19, 2024 12:05
Comment on lines 315 to 322
reports/distributions/core.yaml @open-telemetry/collector-contrib-approvers
reports/distributions/contrib.yaml @open-telemetry/collector-contrib-approvers
reports/distributions/core.yaml @open-telemetry/collector-contrib-approvers
reports/distributions/contrib.yaml @open-telemetry/collector-contrib-approvers


## UNMAINTAINED components

receiver/googlecloudspannerreceiver/ @open-telemetry/collector-contrib-approvers
receiver/googlecloudspannerreceiver/ @open-telemetry/collector-contrib-approvers
Copy link
Contributor

Choose a reason for hiding this comment

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

you should not have manual edits to this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

regenerated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Component New component has been sponsored cmd/githubgen never stale Issues marked with this label will be never staled and automatically removed
Projects
None yet
Development

Successfully merging this pull request may close these issues.