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 module #5

Merged
merged 3 commits into from
Feb 25, 2021
Merged

nginx module #5

merged 3 commits into from
Feb 25, 2021

Conversation

seemk
Copy link
Contributor

@seemk seemk commented Feb 15, 2021

Contributing an nginx OpenTelemetry module. For usage refer to README.md.

Supported features:

  • Exporter: OTLP
  • Propagation: W3C, b3
  • Modules: http, fastcgi

Platforms: Linux with nginx 1.18 (need to add tests for more versions)

I would like to get CI working in this contrib repository as well - at the moment I just copied the CI files here, but I guess they have to live in the repo root?

I'd like to keep working on and maintain the module, adding additional platform support, configuration options, and so on. How can we make this happen? Would it be a PR against this repo from a fork each time?

Ideas for changes welcome! 🙂

@seemk seemk mentioned this pull request Feb 15, 2021
@lalitb
Copy link
Member

lalitb commented Feb 17, 2021

I would like to get CI working in this contrib repository as well - at the moment I just copied the CI files here, but I guess they have to live in the repo root?

I think it should be fine to keep component level CI scripts inside their specific folders ( instead of root of repo ), and the workflow jobs referring them from within that.

I'd like to keep working on and maintain the module, adding additional platform support, configuration options, and so on. How can we make this happen? Would it be a PR against this repo from a fork each time?

My initial thoughts are to have codeowners based approach, with codeowners for each of the components responsible for approval/merger, and the current repo approvers / maintainers been added in PR to facilitate the process. At the same time it shouldn't add to extra delay in merges. Would like to hear other views ( @open-telemetry/cpp-approvers @open-telemetry/cpp-maintainers ) and we can further discuss in our community meeting to finalize the approach.

@seemk
Copy link
Contributor Author

seemk commented Feb 22, 2021

I would like to get CI working in this contrib repository as well - at the moment I just copied the CI files here, but I guess they have to live in the repo root?

I think it should be fine to keep component level CI scripts inside their specific folders ( instead of root of repo ), and the workflow jobs referring them from within that.

I'd like to keep working on and maintain the module, adding additional platform support, configuration options, and so on. How can we make this happen? Would it be a PR against this repo from a fork each time?

My initial thoughts are to have codeowners based approach, with codeowners for each of the components responsible for approval/merger, and the current repo approvers / maintainers been added in PR to facilitate the process. At the same time it shouldn't add to extra delay in merges. Would like to hear other views ( @open-telemetry/cpp-approvers @open-telemetry/cpp-maintainers ) and we can further discuss in our community meeting to finalize the approach.

As for CI I put the workflow file into the root dir .github/workflows/nginx.yml which references the CI scripts inside the nginx subdirectory - should be quite clutter free like this.

To get this reviewed and approved, is there anyone I can ping? Thanks 🙂

@lalitb
Copy link
Member

lalitb commented Feb 23, 2021

To get this reviewed and approved, is there anyone I can ping? Thanks 🙂

I would like to get it approved after adding you ( and other you would like to add ) as CODEOWNERS. Was actually waiting for this week SIG community meeting to finalize the approach, as this would be then valid for all future components added in contrib repo. Sorry for long wait for this PR, this should have been decided last week had there been SIG meeting.

@seemk
Copy link
Contributor Author

seemk commented Feb 23, 2021

To get this reviewed and approved, is there anyone I can ping? Thanks slightly_smiling_face

I would like to get it approved after adding you ( and other you would like to add ) as CODEOWNERS. Was actually waiting for this week SIG community meeting to finalize the approach, as this would be then valid for all future components added in contrib repo. Sorry for long wait for this PR, this should have been decided last week had there been SIG meeting.

Sure, no problem. I'll try to join this week as well.

@maxgolov maxgolov self-requested a review February 24, 2021 18:23
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM.

@seemk
Copy link
Contributor Author

seemk commented Feb 25, 2021

FYI I don't have the rights to merge once approved.

@lalitb
Copy link
Member

lalitb commented Feb 25, 2021

FYI I don't have the rights to merge once approved.

I will merge the PR for now. Going forward, we need to have different maintainers and approvers for this repo, as I know not all approvers/maintainers for opentelemetry-cpp will like to own same role for contrib repo. Let me check on our slack channel for volunteers.

Also, had some concern with "MIT License" and non-opentelemery copyright code going in CNCF managed repo ( as here but I think that should be fine as clarified in below CNCF link:

https://github.com/cncf/foundation/blob/master/copyright-notices.md#what-about-third-party-code

If a file only contains code that originates from a third party source who didn't contribute it themselves, then you would not want to add the notices above. (In a similar vein, you wouldn't add a notice identifying you as the copyright holder either, if you didn't own it.) Just preserve the existing copyright and license notices as they are.

@lalitb lalitb merged commit 8a4902c into open-telemetry:main Feb 25, 2021
@marcalff marcalff added the instrumentation:nginx Nginx module label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants