Skip to content
This repository has been archived by the owner on Jul 24, 2023. It is now read-only.

Initial import, plus http status code updates #1

Merged
merged 11 commits into from Dec 1, 2021
Merged

Initial import, plus http status code updates #1

merged 11 commits into from Dec 1, 2021

Conversation

ahobson
Copy link
Contributor

@ahobson ahobson commented Nov 22, 2021

Create a new otelhttp repo.

We aren't importing the entirety of go-opentelemetry-contrib because we want to enforce go linting rules and don't want to have to update the entire repository.

This PR includes changes from open-telemetry/opentelemetry-go-contrib#771

Copy link
Contributor

@felipe-lee felipe-lee left a comment

Choose a reason for hiding this comment

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

I noticed that in the original repo they have a go.mod and go.sum in the test directory that aren't in ours. Should those be included too? If not, maybe a comment in the README indicating that they shouldn't and a reason would be good for future us when we try updating this with any changes they've made.

.golangci.yml Outdated Show resolved Hide resolved
nix/update.sh Outdated Show resolved Hide resolved
Comment on lines +30 to +36
(import (builtins.fetchGit {
# Descriptive name to make the store path easier to identify
name = "act-0.2.24";
url = "https://github.com/NixOS/nixpkgs/";
ref = "refs/heads/nixpkgs-unstable";
rev = "a1de1fc28b27da87a84a0b4c9128972ac4a48193";
}) {}).act
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pretty cool, I didn't realize this was a thing. Should we have something in the README to showcase using it, or a script (or Makefile, though I know there's plenty of issues with those) that will run it for us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added something to the README

[otelhttp](https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/instrumentation/net/http/otelhttp)

as of [v.0.26.1](https://github.com/open-telemetry/opentelemetry-go-contrib/commit/7876cd14dc5f09765205caa0fb420fafe23141aa)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add setup instructions? Especially for non-nix users since they'll have to make sure go and pre-commit are set up correctly. Well even nix users will need to install pre-commit hooks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a blurb to the README

@felipe-lee
Copy link
Contributor

Oh also, I don't really know much about licenses. I know we pulled this into it's own repo because of the license difference and that several of the files have the license blurb at the top of the file, but do we also need to include a repo-level license?

@ahobson
Copy link
Contributor Author

ahobson commented Nov 30, 2021

Oh also, I don't really know much about licenses. I know we pulled this into it's own repo because of the license difference and that several of the files have the license blurb at the top of the file, but do we also need to include a repo-level license?

Thank you. I copied the LICENSE from the original repo

@ahobson
Copy link
Contributor Author

ahobson commented Nov 30, 2021

I noticed that in the original repo they have a go.mod and go.sum in the test directory that aren't in ours. Should those be included too? If not, maybe a comment in the README indicating that they shouldn't and a reason would be good for future us when we try updating this with any changes they've made.

README updated!

Copy link
Contributor

@felipe-lee felipe-lee left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for making those updates. LGTM!

Copy link

@gidjin gidjin left a comment

Choose a reason for hiding this comment

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

LGTM. One question this repo doesn't look like a fork of the original, is there a reason we couldn't have done that?

@felipe-lee
Copy link
Contributor

LGTM. One question this repo doesn't look like a fork of the original, is there a reason we couldn't have done that?

@gidjin I think that's what @ahobson was mentioning with this:

We aren't importing the entirety of go-opentelemetry-contrib because we want to enforce go linting rules and don't want to have to update the entire repository.

So I think the goal is by only keeping up with a single package it can make it easier to maintain. Though I guess one question is, how we plan on keeping up to date. Though hopefully we won't need to for too long

@ahobson
Copy link
Contributor Author

ahobson commented Dec 1, 2021

LGTM. One question this repo doesn't look like a fork of the original, is there a reason we couldn't have done that?

@gidjin I think that's what @ahobson was mentioning with this:

We aren't importing the entirety of go-opentelemetry-contrib because we want to enforce go linting rules and don't want to have to update the entire repository.

So I think the goal is by only keeping up with a single package it can make it easier to maintain. Though I guess one question is, how we plan on keeping up to date. Though hopefully we won't need to for too long

I mean, the reason for the fork is that the folks are not updating the original with PRs we need. It seems to me like we'll have one of two outcomes:

  1. They start accepting PRs and so we can stop using this fork
  2. The original is basically abandonware and so we never have to worry about keeping up to date

@ahobson ahobson merged commit a6d8db5 into main Dec 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants