Skip to content

fix: apply //dd:orchestrion-enabled to tracer-internal modules#223

Merged
RomainMuller merged 6 commits into
mainfrom
nick.ripley/orchestrion-enabled-internal
Aug 22, 2024
Merged

fix: apply //dd:orchestrion-enabled to tracer-internal modules#223
RomainMuller merged 6 commits into
mainfrom
nick.ripley/orchestrion-enabled-internal

Conversation

@nsrip-dd
Copy link
Copy Markdown
Contributor

What does this PR do?

The //dd:orchestrion-enabled directive aspect is currently not applied to
tracer-internal modules. dd-trace-go will rely on this directive to detect
whether it was built with Orchestrion. So, set "tracer-internal: true" for the
directive.

Nothing seems to be broken right now. I'm not sure the best way to test this, at
least in this PR. I'm working on an integration test for the profiler when
built with Orchestrion. Once that's up and running, it will break if the
profiler relies on the directive to add Orchestrion-related info. And
presumably once we start relying on goroutine-local storage for context, stuff
will break if the directive doesn't work in dd-trace-go?

Motivation

I happened to notice this was broken while trying to add Orchestrion-related
information to the profiler uploads. The internal/orchestrion.Enabled
function in dd-trace-go was always returning false, even when built with
Orchestrion.

Reviewer's Checklist

  • Changed code has unit tests for its functionality.

The //dd:orchestrion-enabled directive aspect is currently not applied
to tracer-internal modules. dd-trace-go will rely on this directive to
detect whether it was built with Orchestrion. So, set "tracer-internal:
true" for the directive.

I happened to notice this was broken while trying to add
Orchestrion-related information to the profiler uploads. The
`internal/orchestrion.Enabled` function in dd-trace-go was always
returning false, even when built with Orchestrion.
@nsrip-dd nsrip-dd changed the title Apply //dd:orchestrion-enabled to tracer-internal modules fix: apply //dd:orchestrion-enabled to tracer-internal modules Aug 15, 2024
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.82%. Comparing base (0922cdc) to head (bc3c198).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #223   +/-   ##
=======================================
  Coverage   61.82%   61.82%           
=======================================
  Files          93       93           
  Lines        5027     5027           
=======================================
  Hits         3108     3108           
  Misses       1592     1592           
  Partials      327      327           
Flag Coverage Δ
ARM64 45.16% <ø> (ø)
Linux 66.59% <ø> (ø)
Windows 43.83% <ø> (ø)
X64 61.82% <ø> (ø)
generator 42.71% <ø> (ø)
go 51.52% <ø> (ø)
go1.22 45.16% <ø> (ø)
go1.23 44.94% <ø> (ø)
integration 46.26% <ø> (ø)
macOS 45.16% <ø> (ø)
unit 40.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nsrip-dd
Copy link
Copy Markdown
Contributor Author

@eliottness The integration tests are failling with this change. Example. I think it's due to this part in dd-trace-go:

type contextStack map[any][]any

// getDDContextStack is a main way to access the GLS slot of runtime.g inserted by orchestrion. This function should not be
// called if the enabled variable is false.
func getDDContextStack() *contextStack {
	if gls := getDDGLS(); gls != nil {
		return gls.(*contextStack)
	}

	newStack := new(contextStack)
	setDDGLS(newStack)
	return newStack
}

The new(contextStack) call gives you a non-nil pointer to a nil map. So the methods later check that the receiver s *contextStack is not nil, but that just means the pointer isn't nil. There needs to be a make call to actually create the map. I think adding *newStack = make(contextStack) would work?

nsrip-dd added a commit that referenced this pull request Aug 15, 2024
If the log level is set via `--log-level`, set the corresponding
environment variables so that the child processes will get it. This was
done for `--log-file` already, but only if it was originally a relative
path. Propagate it always.

I noticed this when investigating #223. I was setting the log level to
"trace" via the command line and didn't see much output. It took me a
bit to realize that the setting wasn't picked up by all the workers. I
set the env var instead, but ideally the command line argument works the
same way :)
RomainMuller pushed a commit that referenced this pull request Aug 16, 2024
### What does this PR do?

If the log level is set via `--log-level`, set the corresponding
environment
variables so that the child processes will get it. This was done for
`--log-file` already, but only if it was originally a relative path.
Propagate
it always.

### Motivation

I noticed this when investigating #223. I was setting the log level to
"trace"
via the command line and didn't see much output. It took me a bit to
realize
that the setting wasn't picked up by all the workers. I set the env var
instead, but ideally the command line argument works the
same way :)

### Reviewer's Checklist

- [ ] Changed code has unit tests for its functionality.
@nsrip-dd nsrip-dd marked this pull request as ready for review August 16, 2024 12:44
@nsrip-dd nsrip-dd requested a review from a team as a code owner August 16, 2024 12:44
@RomainMuller
Copy link
Copy Markdown
Contributor

RomainMuller commented Aug 16, 2024

This particular fix is also present in #225, which also includes a monkey-patch for the current issue in dd-trace-go.

…on-enabled-internal

# Conflicts:
#	_integration-tests/go.mod
#	_integration-tests/go.sum
#	go.mod
#	go.sum
#	internal/injector/builtin/generated.go
#	samples/go.mod
#	samples/go.sum
@RomainMuller RomainMuller enabled auto-merge August 20, 2024 08:47
@RomainMuller
Copy link
Copy Markdown
Contributor

This actually needs #232 before the CI will consistently pass; it is currently flaky due to re-use of improperly cleaned up g objects.

@RomainMuller RomainMuller changed the title fix: apply //dd:orchestrion-enabled to tracer-internal modules fix: apply //dd:orchestrion-enabled to tracer-internal modules Aug 22, 2024
…on-enabled-internal

# Conflicts:
#	internal/injector/builtin/generated.go
@RomainMuller RomainMuller added this pull request to the merge queue Aug 22, 2024
@RomainMuller RomainMuller removed this pull request from the merge queue due to a manual request Aug 22, 2024
@RomainMuller RomainMuller enabled auto-merge August 22, 2024 09:56
@RomainMuller RomainMuller added this pull request to the merge queue Aug 22, 2024
Merged via the queue into main with commit 4420b75 Aug 22, 2024
@RomainMuller RomainMuller deleted the nick.ripley/orchestrion-enabled-internal branch August 22, 2024 10:56
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.

3 participants