Skip to content

Embed templates inside dd agent binary#4782

Merged
prognant merged 6 commits intomasterfrom
prognant/embed-templates-insidde-dd-agent-binary
Feb 7, 2020
Merged

Embed templates inside dd agent binary#4782
prognant merged 6 commits intomasterfrom
prognant/embed-templates-insidde-dd-agent-binary

Conversation

@prognant
Copy link
Copy Markdown
Contributor

What does this PR do?

It embeds templates files into the agent binary at compilation time.

Motivation

It makes the agent binary able to run agent status without requiring
to the original template file.

Additional Notes

It relies on go-bindata that has been added to the dependencies.

@prognant prognant added kind/enhancement do-not-merge/WIP [deprecated] team/agent-core Deprecated. Use metrics-logs / shared-components labels instead.. labels Jan 24, 2020
@prognant prognant added this to the 7.18.0 milestone Jan 24, 2020
@prognant prognant requested review from a team as code owners January 24, 2020 18:09
@prognant prognant force-pushed the prognant/embed-templates-insidde-dd-agent-binary branch 2 times, most recently from 9448008 to f3c70d2 Compare January 24, 2020 23:07
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 25, 2020

Codecov Report

Merging #4782 into master will increase coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4782      +/-   ##
==========================================
+ Coverage   52.32%   52.39%   +0.06%     
==========================================
  Files         709      709              
  Lines       51004    50947      -57     
==========================================
+ Hits        26689    26692       +3     
+ Misses      22735    22676      -59     
+ Partials     1580     1579       -1
Flag Coverage Δ
#linux 53.63% <0%> (+0.05%) ⬆️
#windows 52.79% <0%> (+0.08%) ⬆️
Impacted Files Coverage Δ
pkg/status/render.go 0% <0%> (-1.24%) ⬇️
pkg/collector/scheduler/scheduler.go 84.66% <0%> (+0.61%) ⬆️
pkg/logs/auditor/auditor.go 71.83% <0%> (+0.7%) ⬆️
pkg/secrets/secrets.go 69.84% <0%> (+2.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6515895...f3c70d2. Read the comment docs.

It relies on go-bindata and a go:generate statement to create a go
file out of the template files so they are not required anymore at
runtime.
@prognant prognant force-pushed the prognant/embed-templates-insidde-dd-agent-binary branch from f3c70d2 to 1396e4e Compare January 27, 2020 09:19
@prognant prognant requested a review from a team as a code owner January 27, 2020 09:19
@prognant prognant changed the title Prognant/embed templates insidde dd agent binary Embed templates inside dd agent binary Jan 27, 2020
Copy link
Copy Markdown
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

This looks very clean to me! Just a comment on the go-bindata tool we'll be using: we should pin it.

Comment thread bootstrap.json Outdated
Comment thread tasks/__init__.py
ns.add_task(audit_tag_impact)
ns.add_task(e2e_tests)
ns.add_task(make_kitchen_gitlab_yml)
ns.add_task(generate)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was wondering if agent:generate wasn't a better invoke target, but I realized, also from your changes, that generate may be required by other tasks in the main collection too, so I guess putting it here is the sensible thing to do.

@gzussa gzussa requested a review from a team January 30, 2020 12:48
truthbk
truthbk previously approved these changes Jan 30, 2020
Copy link
Copy Markdown
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

Thank you! Nice job!

KSerrania
KSerrania previously approved these changes Jan 31, 2020
Copy link
Copy Markdown
Contributor

@KSerrania KSerrania left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread tasks/go.py Outdated
Comment thread tasks/go.py Outdated
Comment thread tasks/go.py
ctx.run("rm -rf ./vendor")

@task
def generate(ctx):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit for later: the name feels a bit too generic for what this function does: for instance, it's difficult to understand without going into the specifics of the function that the:

def build(...):
  ...
  # Generate go source from template
  generate(ctx)
  ...

line in tasks/agent.py means we apply go generate on ./pkg/status.

Moreover, we use go generate in some other places (such as tasks/cluster_agent.py:

cmd = "go generate -tags '{build_tags}' {repo_path}/cmd/cluster-agent"
ctx.run(cmd.format(build_tags=" ".join(build_tags), repo_path=REPO_PATH), env=env)
, tasks/dogstatsd.py:
cmd = "go generate {}/cmd/dogstatsd"
ctx.run(cmd.format(REPO_PATH), env=env)
, and tasks/trace_agent.py:
ctx.run("go generate {REPO_PATH}/pkg/trace/info".format(**args), env=env)
ctx.run(cmd.format(**args), env=env)
), so we could use this function in these cases too (with some tweaks to take different targets and options).

I'd either:

  • Not change anything, but show explicitly that this function renders only what's specified in GO_GENERATE_TARGETS (eg. by adding a comment around its invocation in tasks/agent.py and tasks/cluster_agent.py) so that someone reading the build tasks understands what's happening without having to read the internal function's behavior),
  • Make this function more generic by accepting a list of targets (and maybe a list of options if needed)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree that it will make sense at some point to write a full-featured wrapper for go generate. In-between I took option #1 and added more meaningful comments.

@prognant prognant dismissed stale reviews from KSerrania and truthbk via a9b1027 January 31, 2020 14:12
KSerrania
KSerrania previously approved these changes Jan 31, 2020
Comment thread .gitignore Outdated
clamoriniere
clamoriniere previously approved these changes Feb 6, 2020
Co-Authored-By: Cedric Lamoriniere <cedric.lamoriniere@datadoghq.com>
@prognant prognant dismissed stale reviews from clamoriniere and KSerrania via d2210a1 February 7, 2020 08:37
@bits-bot
Copy link
Copy Markdown
Collaborator

bits-bot commented Feb 7, 2020

CLA assistant check
All committers have signed the CLA.

@prognant prognant merged commit a97f181 into master Feb 7, 2020
@prognant prognant deleted the prognant/embed-templates-insidde-dd-agent-binary branch February 7, 2020 09:56
@albertvaka
Copy link
Copy Markdown
Contributor

albertvaka commented Feb 7, 2020

I wonder if we could also do this for the html files we use in the agent launch-gui command? I guess not because the embedded browser component needs to be able to find them?

@prognant
Copy link
Copy Markdown
Contributor Author

IMHO that would worth trying but in the meantime we have a windows issue likely related to this change. I will try to follow up on that topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[deprecated] team/agent-core Deprecated. Use metrics-logs / shared-components labels instead.. kind/enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants