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

relocate agent codebase into pkg/signalfx-agent #2717

Merged
merged 5 commits into from
Mar 22, 2023
Merged

relocate agent codebase into pkg/signalfx-agent #2717

merged 5 commits into from
Mar 22, 2023

Conversation

rmfitzpatrick
Copy link
Contributor

@rmfitzpatrick rmfitzpatrick commented Mar 13, 2023

These changes relocate a reduced set of the Smart Agent codebase to this project for local use by the SA receiver and extension. The workflow to land the changes is as follows:

  1. go mod vendor this project and move the SA project to pkg/signalfx-agent, deleting the vendor dir. This process includes elements that are used by the core agent and not necessarily by our components. Deleting dead paths can be a helpful aim in the future but I didn't attempt to tackle it here.
  2. Move all _test.go files from the SA project to pkg/signalfx-agent
  3. Move the minimum set of SA scripts and cmd/monitorcodegen for metadata.yaml genmetadata.go and collectd template.go generation capabilities. edit: The monitorcodegen template was slightly modified to not require updates when running gofmt (removes unnecessary []string declaration for group metric map).
  4. Bring about a minimal Makefile with dev targets*. edit: I've updated these to include noops for the GH actions.
  5. Move .golangci.yaml from the SA project and update its disabled linters to pass for our for-all CMD='make lint' target.

For this PR I request that the comments be limited to the functional effects of the changes. This is not intended to be a forum for SA-wide content suggestions, though improving the quality and coverage for all monitors would be very nice to have. I think it could be helpful to remove the fmt/lint/test wall between the modules over time but the immediate benefits aren't clear to me.

I know this is a large PR but thought it'd be helpful to note these files that weren't directly taken from the agent project:

@rmfitzpatrick rmfitzpatrick requested review from a team as code owners March 13, 2023 19:26
@rmfitzpatrick rmfitzpatrick force-pushed the sfxa branch 2 times, most recently from 735e45b to 5c67098 Compare March 13, 2023 19:57
@dmitryax
Copy link
Contributor

dmitryax commented Mar 13, 2023

It'd be great if we could move the code keeping the git history. It was done for core->contrib components migration open-telemetry/opentelemetry-collector-contrib#4764. So far, it has been pretty helpful in tracking down historical changes. It's just a suggestion tho. Up to the team

@rmfitzpatrick
Copy link
Contributor Author

rmfitzpatrick commented Mar 13, 2023

It'd be great if we could move the code keeping the git history. It was done for core->contrib components migration open-telemetry/opentelemetry-collector-contrib#4764. So far, it has been pretty helpful in tracking down historical changes. It's just a suggestion tho. Up to the team

I'd considered using a subtree but felt the ease and known integrity of relying on go mod vendor would be helpful enough for the aim of dep fulfillment. I can try the former and then restrict the final set to be migrated, but since the history of the agent repo is isolated and when archived the github project will still be available I didn't see a strong motivation if the ultimate goal is git blame. If this is an unreasonable view I'd be interested in other viewpoints.

One unknown for this potential* effort is the agent already having a large migration effort at the end of v4 and git log --follow behavior with twice migrated content: signalfx/signalfx-agent@c295133

@jeffreyc-splunk
Copy link
Contributor

It'd be great if we could move the code keeping the git history. It was done for core->contrib components migration open-telemetry/opentelemetry-collector-contrib#4764. So far, it has been pretty helpful in tracking down historical changes. It's just a suggestion tho. Up to the team

I'd personally be fine if we just document a link to the signalfx-agent commit that this migration is based on.

@jeffreyc-splunk
Copy link
Contributor

jeffreyc-splunk commented Mar 15, 2023

@rmfitzpatrick Can you rebase to pick up the arm64 bundle integration and tests?

@jeffreyc-splunk
Copy link
Contributor

Not required for this PR, but we should update https://github.com/signalfx/splunk-otel-collector/blob/main/.github/dependabot.yml with make gendependabot.

@jeffreyc-splunk
Copy link
Contributor

Probably outside the scope of this PR, but do we plan on migrating the monitor doc processes, e.g. https://github.com/signalfx/signalfx-agent/blob/main/scripts/make-monitor-doc?

@rmfitzpatrick
Copy link
Contributor Author

Probably outside the scope of this PR, but do we plan on migrating the monitor doc processes, e.g. https://github.com/signalfx/signalfx-agent/blob/main/scripts/make-monitor-doc?

Would agree this is out of scope and offhand I'd think that converging on https://github.com/splunk/collector-config-tools using an updated ~gomplate system as a base could be a good target. @pmcollins may have thoughts?

@pmcollins
Copy link
Contributor

Probably outside the scope of this PR, but do we plan on migrating the monitor doc processes, e.g. https://github.com/signalfx/signalfx-agent/blob/main/scripts/make-monitor-doc?

Would agree this is out of scope and offhand I'd think that converging on https://github.com/splunk/collector-config-tools using an updated ~gomplate system as a base could be a good target. @pmcollins may have thoughts?

Yeah, that's fine with me. I try to justify putting things in that repo that at first glance don't appear to belong there by making sure they are used by the tool, but we could definitely add some kind of display capability for the generated docs.

Copy link
Contributor

@jeffreyc-splunk jeffreyc-splunk left a comment

Choose a reason for hiding this comment

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

LGTM. Should we wait to merge until after the v0.73.0 release?

@rmfitzpatrick
Copy link
Contributor Author

I attempted to migrate the histories with the following and am unable to see anything other than the prepare pkg relocation to splunk-otel-collector commit w/ git log --follow:

cd signalfx-agent
git branch -D relocated pkg-relocation
git checkout -b relocated
git clean -fxd
mkdir -p soc/pkg/signalfx-agent/cmd soc/pkg/signalfx-agent/scripts
git mv pkg soc/pkg/signalfx-agent
git mv scripts/collectd-template-to-go soc/pkg/signalfx-agent/scripts
git mv scripts/make-versions soc/pkg/signalfx-agent/scripts
git mv cmd/monitorcodegen soc/pkg/signalfx-agent/cmd
git mv Makefile soc/pkg/signalfx-agent
git mv .gitignore soc/pkg/signalfx-agent
git mv .golangci.yml soc/pkg/signalfx-agent
git commit -m "prepare pkg relocation to splunk-otel-collector" -S
git subtree split -P soc -b pkg-relocation

cd ../splunk-otel-collector
git branch -D sfxawithhistory prep-sfxa-relocation pkg-relocation
git checkout --orphan prep-sfxa-relocation
git pull ../signalfx-agent pkg-relocation
git checkout -b sfxawithhistory
git merge --allow-unrelated-histories prep-sfxa-relocation
git branch -d prep-sfxa-relocation

In the process it became clearer to me that migrating thousands of agent commits to this project is probably not desirable overall.

@rmfitzpatrick rmfitzpatrick merged commit 00543d9 into main Mar 22, 2023
@delete-merged-branch delete-merged-branch bot deleted the sfxa branch March 22, 2023 13:45
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.

5 participants