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

Nginx Instrumentation crd #1853

Merged
merged 13 commits into from
Jun 27, 2023
Merged

Conversation

chrlic
Copy link
Member

@chrlic chrlic commented Jun 18, 2023

Instrumentation crd for Nginx autoinstrumentation

Aside of standard fields, it contains two more:

  • Attrs - allows specific settings for instrumentation library, comment points to the doc and will go into README.md eventually
  • ConfigFile - allows to specify custom configuration file location for Nginx if it's different from most used /etc/nginx/nginx.conf. This is needed since the instrumentation essentially modifies this file to load the instrumentation shared library and to set OpenTelemetry parameters.

In a nutshell, Nginx instrumentation follows similar methods to Apache HTTPD instrumentation.

@chrlic chrlic requested a review from a team June 18, 2023 09:04
@chrlic chrlic marked this pull request as draft June 18, 2023 09:32
@chrlic chrlic marked this pull request as ready for review June 18, 2023 17:42
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

Cool to see more auto-instrumentation support :)

Could you please share the link where the nginx instrumentation is hosted?

type Nginx struct {
// Image is a container image with Nginx SDK and auto-instrumentation.
// +optional
Image string `json:"image,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Is there already image for nginx instrumentation?

Copy link
Member

Choose a reason for hiding this comment

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

ah it's here #1852

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

For Nginx autoinstrumentation, default image with the instrumentation libraries is the same as for Apache Httpd - autoinstrumentation-apache-httpd

@pavolloffay
Copy link
Member

The PR needs rebasing

}
if r.Spec.Nginx.Resources.Limits == nil {
r.Spec.Nginx.Resources.Limits = corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("500m"),
Copy link
Member

Choose a reason for hiding this comment

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

we should extract these to a constant and reuse for others.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@chrlic chrlic closed this Jun 27, 2023
@chrlic chrlic force-pushed the nginx-a17n-crd branch 2 times, most recently from 18b51fc to a865360 Compare June 27, 2023 11:48
@chrlic
Copy link
Member Author

chrlic commented Jun 27, 2023

rebased, reopenning

@chrlic chrlic reopened this Jun 27, 2023
@pavolloffay pavolloffay merged commit 3b26276 into open-telemetry:main Jun 27, 2023
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* Nginx Instrumentation crd

* Nginx Instruemtnation crd

* chloggen

* typos

* make generate crd

* make bundle - crd

* make bundle - versions

* default resources

* consts for resource settings

* rename resource consts

* rename resource consts
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.

2 participants