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

Redo packages and directories #314

Conversation

newrelic-eheinlein
Copy link

@newrelic-eheinlein newrelic-eheinlein commented Aug 28, 2020

Addresses #164 to add a directory to each contrib packages to include "otel{PACKAGE}" in the name.

Resolves #164

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 28, 2020

CLA Check
The committers are authorized under a signed CLA.

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this, it looks tedious. There are some details I think we need to address still.

WORKDIR needs to be updated (couldn't comment on a zero change file) in DOCKERFILE:

  • opentelemetry-go-contrib/instrumentation/github.com/Shopify/sarama/otelsarama/example/producer/Dockerfile
  • opentelemetry-go-contrib/instrumentation/github.com/emicklei/go-restful/otelrestful/example/Dockerfile
  • opentelemetry-go-contrib/instrumentation/github.com/gin-gonic/gin/otelgin/example/Dockerfile
  • opentelemetry-go-contrib/instrumentation/github.com/gorilla/mux/otelmux/example/Dockerfile
  • opentelemetry-go-contrib/instrumentation/github.com/labstack/echo/otelecho/example/Dockerfile
  • opentelemetry-go-contrib/instrumentation/gopkg.in/macaron.v1/otelmacaron/example/Dockerfile
  • opentelemetry-go-contrib/instrumentation/net/http/httptrace/otelhttptrace/example/Dockerfile
  • opentelemetry-go-contrib/instrumentation/net/http/otelhttp/example/Dockerfile

Also, the runtime package looks to have been renamed, but I thought the goal was to keep this preserved.

instrumentation/README.md Outdated Show resolved Hide resolved
- `go.opentelemetry.io/contrib/instrumentation/database/sql`
- `go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/otelmux`
- `go.opentelemetry.io/contrib/instrumentation/gopkg.in/macaron.v1/otelmacaron`
- `go.opentelemetry.io/contrib/instrumentation/database/sql/otelsql`

Consequentially, this means that all instrumentation MUST be contained in a sub-directory structure matching the package name.
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to add the caveat about things like the runtime package that doesn't instrument any particular package and how they are the exception to the rule.

// trace and metrics to beego web apps.
type OTelBeegoHandler struct {
type BeegoHandler struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there a conflict that motivated this change?

Comment on lines +1 to +14
module go.opentelemetry.io/contrib/instrumentation/macaron/otelmacaron

go 1.14

replace go.opentelemetry.io/contrib => ../../../..

require (
github.com/stretchr/testify v1.6.1
go.opentelemetry.io/contrib v0.11.0
go.opentelemetry.io/otel v0.11.0
go.opentelemetry.io/otel/exporters/stdout v0.11.0
go.opentelemetry.io/otel/sdk v0.11.0
gopkg.in/macaron.v1 v1.3.9
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that this was empty. This looks like a bug fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, looks like git thought the old version was moved to another package.

@@ -1,13 +0,0 @@
module go.opentelemetry.io/contrib/instrumentation/runtime
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this file was erroneous deleted.

@@ -29,4 +29,4 @@
// runtime.go.mem.heap_sys (bytes) Bytes of heap memory obtained from the OS
// runtime.go.mem.live_objects - Number of live objects is the number of cumulative Mallocs - Frees
// runtime.uptime (ms) Milliseconds since application was initialized
package runtime // import "go.opentelemetry.io/contrib/instrumentation/runtime"
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the plan was to not change the runtime package given it does not instrument a package. Was there a different decision made?

@@ -1,4 +1,4 @@
module go.opentelemetry.io/contrib/instrumentation/macaron
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this looks like git interpreted the macaron go.mod to be moved here which looks incorrect. This change graph should be fixed.

@MrAlias MrAlias added this to Needs Triage in GA Burndown Sep 3, 2020
@MrAlias MrAlias added area: instrumentation Related to an instrumentation package priority:p1 labels Sep 3, 2020
@MrAlias MrAlias moved this from Needs Triage to In progress in GA Burndown Sep 3, 2020
@MrAlias MrAlias moved this from In progress to Review in progress in GA Burndown Sep 3, 2020
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@MrAlias
Copy link
Contributor

MrAlias commented Sep 23, 2020

Going to give this another go instead of updating given the large number of conflicts.

@MrAlias MrAlias closed this Sep 23, 2020
GA Burndown automation moved this from Review in progress to Done Sep 23, 2020
@MrAlias MrAlias added this to Done in OpenTelemetry Go RC Oct 13, 2020
plantfansam referenced this pull request in plantfansam/opentelemetry-go-contrib Mar 18, 2022
* clean dead code from noop tracer

* rename arguments from the Tracer interface's methods

* remove service, resources and component options from sdk/tracer and mock tracer

* remove unused fields from sdk/tracer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: instrumentation Related to an instrumentation package
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Update instrumentation naming conventions
3 participants