Cloud metadata by eyalkoren · Pull Request #1599 · elastic/apm-agent-java

eyalkoren · 2021-01-06T11:13:44Z

What does this PR do?

Closes #1264
Extending the work started at #1453

Checklist

ghost · 2021-01-06T11:28:19Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Started by user eyalkoren
- Start Time: 2021-01-26T14:26:53.523+0000
Duration: 44 min 30 sec
Commit: f6115f3

Test stats 🧪

Test	Results
Failed	0
Passed	1697
Skipped	12
Total	1709

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	1697
Skipped	12
Total	1709

codecov-io · 2021-01-06T11:41:24Z

Codecov Report

Merging #1599 (f6115f3) into master (5e6287a) will increase coverage by 0.52%.
The diff coverage is 81.61%.

@@             Coverage Diff              @@
##             master    #1599      +/-   ##
============================================
+ Coverage     58.77%   59.30%   +0.52%     
- Complexity       92     3462    +3370     
============================================
  Files           399      403       +4     
  Lines         17864    18239     +375     
  Branches       2475     2512      +37     
============================================
+ Hits          10500    10817     +317     
- Misses         6641     6663      +22     
- Partials        723      759      +36

Impacted Files	Coverage Δ	Complexity Δ
.../apm/agent/report/serialize/PayloadSerializer.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...a/co/elastic/apm/agent/log/shipper/FileTailer.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...astic/apm/agent/log/shipper/LogShipperFactory.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...co/elastic/apm/agent/log/shipper/TailableFile.java	`71.06% <ø> (ø)`	`47.00 <0.00> (+47.00)`
.../elastic/apm/agent/impl/CloudMetadataProvider.java	`64.17% <64.17%> (ø)`	`21.00 <21.00> (?)`
...va/co/elastic/apm/agent/impl/ElasticApmTracer.java	`66.25% <66.66%> (+0.21%)`	`81.00 <0.00> (+81.00)`
...nt/configuration/ApmServerConfigurationSource.java	`76.08% <75.00%> (-1.70%)`	`22.00 <1.00> (+22.00)`	⬇️
.../co/elastic/apm/agent/util/UrlConnectionUtils.java	`75.00% <75.00%> (ø)`	`1.00 <1.00> (?)`
.../main/java/co/elastic/apm/agent/impl/MetaData.java	`90.47% <87.50%> (-4.53%)`	`10.00 <2.00> (+10.00)`	⬇️
.../java/co/elastic/apm/agent/util/ExecutorUtils.java	`71.05% <87.50%> (+10.67%)`	`8.00 <3.00> (+8.00)`
... and 15 more

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 5e6287a...f6115f3. Read the comment docs.

felixbarny · 2021-01-20T12:50:05Z

...gent-core/src/main/java/co/elastic/apm/agent/configuration/ApmServerConfigurationSource.java

            return null;
        }
        try {
+            payloadSerializer.blockUntilReady();


Not saying this doesn't work but did you consider providing a Future<MetaData> in the constructor and blocking on Future#get here and call DslJsonSerializer#serializeMetadata(MetaData metaData)? That way, we could keep state management out of the DslJsonSerializer.

Of course, this is how I started, but there were several reasons I changed that: the first is that nobody really needs the metadata object, it is spread around just so it can be handed to serialization. Spreading a Future made even less sense.
In addition, I wanted to create a central serialization for it, meaning that MetaData (or something else) needs to serialize and hold the serialized data. This would mean that MetaData (or something else) now either create and use a PayloadSerializer, or I extract the metadata serialization from the rest, but that proved to be too painful and risky for this PR. Both options looked bad when I tried them, worse then doing it internally within the serializer.
Lastly, blocking during serialization itself doesn't seem right, for example- you better only start the HTTP request when ready to write to it. So I thought it would be better if the serialization interface provides a good indication for that, instead of relying on Future#get (this is not the only usage of it) that each serializer user needs to figure out.

Bottom line - there are many options to do that and I played with several ending with this. Given the multiple dependencies we had on MetaData and the time I wanted to spend on this, this seems as the least ugly.

Thanks for explaining the background

[minor] maybe add a small comment why we do that here in the "least ugly" way

I don't mind, but I think it is weird to explain why something was chosen over something else you wouldn't necessarily consider as a default option. Imagine you only see this code now for the first time- MetaData can be easily an internal serialization thing not known to the outer world (it is not package private only not to broaden the refactoring of this PR even more). In such case, this is the interface of the serializer- it needs time to fully initialize, no matter why, so users of it should be aware. I made efforts to emphasize this in the API as much as possible, including javadoc and an Exception which is part of the API.

SylvainJuge

Nice! we can definitely see that having an asynchronous fetching of metadata is not without complications.

SylvainJuge · 2021-01-21T10:48:28Z

...gent-core/src/main/java/co/elastic/apm/agent/configuration/ApmServerConfigurationSource.java

            return null;
        }
        try {
+            payloadSerializer.blockUntilReady();


[minor] maybe add a small comment why we do that here in the "least ugly" way

apm-agent-core/src/main/java/co/elastic/apm/agent/report/serialize/DslJsonSerializer.java

SylvainJuge · 2021-01-21T11:05:56Z

apm-agent-core/src/main/java/co/elastic/apm/agent/report/serialize/PayloadSerializer.java


    void writeBytes(byte[] bytes, int len);
+
+    class UninitializedException extends Exception {


I would make this a sub-class of IllegalStateException and not checked exception as it would be thrown only when there is a bug (we didn't waited enough or there was an error), it's not a recoverable error like an IOException .

The main purpose of this type is to make anyone calling methods that throws it aware of the requirement for initialization, and the javadoc explains that. I prefer you notice this in compilation and for that it needs to be a checked Exception.

it's not a recoverable

Not sure why - it may affect only the first send/serialization and not the subsequent ones.

apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java

apm-agent-core/src/test/java/co/elastic/apm/agent/impl/MetaDataTest.java

...apm-kafka-headers-plugin/src/test/java/co/elastic/apm/agent/kafka/KafkaClientVersionsIT.java

SylvainJuge · 2021-01-21T12:25:19Z

...m-log-shipper-plugin/src/main/java/co/elastic/apm/agent/log/shipper/ApmServerLogShipper.java


    @Override
-    public boolean onLineAvailable(TailableFile tailableFile, byte[] line, int offset, int length, boolean eol) throws IOException {
+    public boolean onLineAvailable(TailableFile tailableFile, byte[] line, int offset, int length, boolean eol) throws Exception {


[question] why change to the broad Exception here ? If it's a way to make uses of this API to having to handle potential exceptions, that would be worth to add the the method Javadoc to be explicit.

Because it felt inappropriate to add UninitializedException (serializer-specific) to the FileChangeListener interface...
Would you prefer that? Or something else?

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/CloudMetadataProvider.java

SylvainJuge · 2021-01-21T12:33:37Z

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/CloudMetadataProvider.java

+                        executor.shutdown();
+                    }
+
+                    long futureTimeout = queryTimeoutMs + 200;


[question] why do we add a fixed extra timeout for the future, how do we know that 200ms is fine ?

why do we add a fixed extra timeout for the future

We want to limit this blocking somehow, but if I use the same timeout used for the HTTP query, that would be too tight, it needs to be longer.

how do we know that 200ms is fine ?

We don't know, we live on the edge 😛

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/CloudMetadataProvider.java

videnkz and others added 18 commits November 13, 2020 01:33

fetch cloud metada - common logic

9b6a869

implement aws metadata fetch

4ddc982

added gcp metadata parse

712c25b

added azure metadata parse

daba9be

added cloud metadata serialization

46fb991

minor fixes, added test for gcp deserialize

313fbe3

minor fixes

5a348e6

added comma in serialization

6ea891f

fixes with serialization, added tests

02c49d2

deleted mock aws url

6ece6f4

added name to account

a8aad17

added ascii docs

a657c21

deleted todo

717c34c

Merge remote-tracking branch 'upstream/master' into cloud-metadata

6d922a4

Merge remote-tracking branch 'upstream/master' into cloud-metadata

ee66a76

Merge remote-tracking branch 'upstream/master' into cloud-metadata

d4ff4db

Cloud Metadata discovery

c993702

Merge remote-tracking branch 'upstream/master' into cloud-metadata

bf41973

github-actions bot added the agent-java label Jan 6, 2021

Updating configuation.asciidoc

e5b9fde

Supporting tests running within supported clouds

084d22a

eyalkoren requested a review from SylvainJuge January 6, 2021 15:28

eyalkoren added 5 commits January 7, 2021 08:51

Addition to javadoc

58476a3

Merge remote-tracking branch 'upstream/master' into cloud-metadata

3d93edf

Do not log cloud metadata API full response

02f88a2

Try AWS Metadata fetch if token lookup fails

fa00481

GRPC test indentation

d7cb367

eyalkoren marked this pull request as ready for review January 18, 2021 14:20

eyalkoren added 2 commits January 19, 2021 14:41

Fix typo

e21da04

Fix leftovers

25ca669

felixbarny reviewed Jan 20, 2021

View reviewed changes

SylvainJuge reviewed Jan 21, 2021

View reviewed changes

Apply review suggestions

ec8efb5

SylvainJuge approved these changes Jan 26, 2021

View reviewed changes

eyalkoren added 2 commits January 26, 2021 15:47

Merge remote-tracking branch 'upstream/master' into cloud-metadata

d3cdf04

Adding to CHANGELOG

f6115f3

eyalkoren merged commit b9e6d92 into elastic:master Jan 26, 2021

eyalkoren deleted the cloud-metadata branch January 26, 2021 15:44

eyalkoren mentioned this pull request Feb 2, 2021

Avoid sending null fields in the cloud metadata #1648

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cloud metadata#1599

Cloud metadata#1599
eyalkoren merged 30 commits intoelastic:masterfrom
eyalkoren:cloud-metadata

eyalkoren commented Jan 6, 2021 •

edited by zube bot

Loading

ghost commented Jan 6, 2021 •

edited by ghost

Loading

Build stats

Test stats 🧪

Test stats 🧪

codecov-io commented Jan 6, 2021 •

edited

Loading

felixbarny Jan 20, 2021

eyalkoren Jan 20, 2021 •

edited

Loading

felixbarny Jan 20, 2021

SylvainJuge Jan 21, 2021

eyalkoren Jan 21, 2021

SylvainJuge left a comment

SylvainJuge Jan 21, 2021

Uh oh!

SylvainJuge Jan 21, 2021

eyalkoren Jan 21, 2021

Uh oh!

Uh oh!

Uh oh!

SylvainJuge Jan 21, 2021

eyalkoren Jan 21, 2021

Uh oh!

SylvainJuge Jan 21, 2021

eyalkoren Jan 21, 2021

Uh oh!

Labels

5 participants


		void writeBytes(byte[] bytes, int len);

		class UninitializedException extends Exception {

Conversation

eyalkoren commented Jan 6, 2021 • edited by zube bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Checklist

ghost commented Jan 6, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

Test stats 🧪

codecov-io commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Choose a reason for hiding this comment

eyalkoren Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SylvainJuge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Labels

5 participants

eyalkoren commented Jan 6, 2021 •

edited by zube bot

Loading

ghost commented Jan 6, 2021 •

edited by ghost

Loading

codecov-io commented Jan 6, 2021 •

edited

Loading

eyalkoren Jan 20, 2021 •

edited

Loading