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

build: Update DEB and RPM artifacts to align with documentation/non contrib collector #3123

Merged
merged 2 commits into from
Apr 22, 2021

Conversation

Jammicus
Copy link
Contributor

@Jammicus Jammicus commented Apr 15, 2021

Description: Updating RPM and DEB artifacts to match documentation

Currently the RPM and Deb artifacts are not aligned to how they are described in the documentation. They are:

This PR does the following:

  • Bundles the default configuration file in both the RPM and DEB artifacts as
  • Bundles a default environment file with OTELCOL_OPTIONS
  • Removes now redundant test that checks whether the service will crash without a config file. As its bundled with the artifacts it should never be without a config file unless the users remove it. (Happy to add back if you feel its needed)

Link to tracking Issue:
#3097
#3143

Testing: Automated tests have installed + checked service is running after 5 seconds. Manually tested the DEB on Ubuntu 18.04

Documentation: N/A

Edit: Updating path.
Edit 2: Including #3143 as its dependent on the config being present

@codecov
Copy link

codecov bot commented Apr 15, 2021

Codecov Report

Merging #3123 (2a98f6c) into main (81dc441) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3123      +/-   ##
==========================================
+ Coverage   91.59%   91.61%   +0.01%     
==========================================
  Files         486      486              
  Lines       23519    23519              
==========================================
+ Hits        21543    21546       +3     
+ Misses       1466     1464       -2     
+ Partials      510      509       -1     
Flag Coverage Δ
integration 63.35% <ø> (ø)
unit 90.61% <ø> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
internal/stanza/receiver.go 100.00% <0.00%> (+5.88%) ⬆️

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 81dc441...2a98f6c. Read the comment docs.

@pranavmarla
Copy link

pranavmarla commented Apr 15, 2021

@Jammicus Thanks for looking into this!

Quick question: Since this is the contrib collector, should the config be located at:
/etc/otel-collector/config.yaml
OR
/etc/otel-contrib-collector/config.yaml?


Update: Just confirmed by looking at the agent error message in my issue (#3097) -- when the agent fails due to a missing config file, the error message specifically mentions that it's looking for the config file in /etc/otel-contrib-collector/config.yaml. Since it seems to be looking there by default, I assume that's where the default config file should be located.

@Jammicus
Copy link
Contributor Author

Jammicus commented Apr 15, 2021

@Jammicus Thanks for looking into this!

Quick question: Since this is the contrib collector, should the config be located at:
/etc/otel-collector/config.yaml
OR
/etc/otel-contrib-collector/config.yaml?

Update: Just confirmed by looking at the agent error message in my issue (#3097) -- when the agent fails due to a missing config file, the error message specifically mentions that it's looking for the config file in /etc/otel-contrib-collector/config.yaml. Since it seems to be looking there by default, I assume that's where the default config file should be located.

Oooh good spot! Will update

@Jammicus
Copy link
Contributor Author

Looking at the failing tests, its failing due to a test which explicitly checks that the collector does not start without config:

# test install without config, service should not be running
echo
$docker_run
install_pkg $container_name "$PKG_PATH"
sleep 5
echo "Checking $SERVICE_NAME service status after install without config ..."
if $docker_exec systemctl --no-pager status $SERVICE_NAME; then
    echo "$SERVICE_NAME service is running when it should not" >&2
    exit 1
fi
echo "$SERVICE_NAME service is correctly not running"

Looking at the file this seems to have been added when it was initially created. Will need some guidance from the contributors as to whether this is intentional and the collector should not be shipped with default config or can be removed (Happy to update/close this depending on your guidance)

@Jammicus Jammicus changed the title build: Add example config to deb and rpm artifacts [WIP] build: Add example config to deb and rpm artifacts Apr 16, 2021
@Jammicus Jammicus changed the title [WIP] build: Add example config to deb and rpm artifacts [WIP] build: Update DEB and RPM artifacts to align with documentation/non contrib collector Apr 16, 2021
@Jammicus Jammicus changed the title [WIP] build: Update DEB and RPM artifacts to align with documentation/non contrib collector build: Update DEB and RPM artifacts to align with documentation/non contrib collector Apr 16, 2021

# Command-line options for the otel-collector service.
# Run `/usr/bin/otelcontribcol --help` to see all available options.
OTELCOL_OPTIONS="--config=/etc/otel-contrib-collector/config.yaml"

Choose a reason for hiding this comment

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

Thanks again @Jammicus !

Q: I've seen the use of OTELCONTRIBCOL in other places, so not sure what the appropriate contrib naming scheme is for this variable -- should it be OTELCOL_OPTIONS or OTELCONTRIBCOL_OPTIONS?

Also, minor edit: In the comment on line 3, otel-collector should be otel-contrib-collector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated line 3.

Not sure on the naming conventions - approvers can give us some guidance when they have time to review this

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's value in keeping the environment variables symmetrical between contrib and non-contrib, they're mostly just different versions of the same thing so more swappability is nice

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, I would use OTELCONTRIBCOL_OPTIONS just to be consistent with the otelcontribcol binary name.

@bogdandrutu
Copy link
Member

@jchengsfx can you review?


# Command-line options for the otel-collector service.
# Run `/usr/bin/otelcontribcol --help` to see all available options.
OTELCOL_OPTIONS="--config=/etc/otel-contrib-collector/config.yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's value in keeping the environment variables symmetrical between contrib and non-contrib, they're mostly just different versions of the same thing so more swappability is nice

@bogdandrutu
Copy link
Member

@jchengsfx please approve if everything looks good

@bogdandrutu bogdandrutu merged commit f56f902 into open-telemetry:main Apr 22, 2021
pmatyjasek-sumo pushed a commit to pmatyjasek-sumo/opentelemetry-collector-contrib that referenced this pull request Apr 28, 2021
…ontrib collector (open-telemetry#3123)

* build: Add example config to deb and rpm artifacts

* build: update systemd service to point to environment file, update tests to reflect bundled default config.
alexperez52 referenced this pull request in open-o11y/opentelemetry-collector-contrib Aug 18, 2021
…ed) (#3123)

The comment was about traceid/spanid "bytes" fields. This comment is incorrect since
we have changed traceid/spanid fields to use hex encoding (as required by the spec).

In the future if we add any other "bytes" fields to the proto they will be encoded as base64
as it is expected and is default behavior of Protobuf/JSON, but this does not require an
explicit comment, so I am deleting it.
mstumpfx pushed a commit to mstumpfx/opentelemetry-collector-contrib that referenced this pull request Aug 31, 2021
…ontrib collector (open-telemetry#3123)

* build: Add example config to deb and rpm artifacts

* build: update systemd service to point to environment file, update tests to reflect bundled default config.
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.

None yet

5 participants