HWKAPM-798 | HTTP recorder allow custom options #30
Conversation
|
@karelhala could you please review? |
pavolloffay
force-pushed
the
HWKAPM-798-http-recorder-custom-options
branch
2 times, most recently
from
f0477cc
to
a46f216
Compare
There was a problem hiding this comment.
Looks good! Just couple of proposals, so if @pavolloffay does not agree on changing Object.assign to spread operator and simplifying fetch methods by removing brackets. I'm fine with it.
| }, | ||
| }; | ||
| this[ENDPOINT] = `${url}/hawkular/apm/traces/fragments`; | ||
| this[HTTP_OPTIONS] = Object.assign({ |
There was a problem hiding this comment.
Object.assign is older, you should now use spread operator, advantage is that it will create new object of options and is somewhat more readable. However it needs to be polyfilled with babel, es6 proposal.
this[HTTP_OPTIONS] = {method: 'POST', ...options}There was a problem hiding this comment.
This looks nicer! I will give it a try.
There was a problem hiding this comment.
You might have to install rest spread - https://babeljs.io/docs/plugins/transform-object-rest-spread/
There was a problem hiding this comment.
I wanted to play with it in browser and it does not work :(. It is still in stage 3 proposal, I will stick with assign..
| headers: this[HTTP_OPTIONS].headers, | ||
| fetch(this[ENDPOINT], Object.assign({}, this[HTTP_OPTIONS], { | ||
| body: JSON.stringify([trace]) | ||
| })).then((response) => { |
There was a problem hiding this comment.
Can be simplified by omiting brackets
...
})).then(response => response.status !== 204 && console.log(`Server did not accept trace data: ${response}`))
...There was a problem hiding this comment.
I will it as it is, makes it easier to extend.
| })).then((response) => { | ||
| if (response.status !== 204) { | ||
| console.log(`Server did not accept trace data: ${response}`); | ||
| } | ||
| }).catch((err) => { |
There was a problem hiding this comment.
Can be simplified by omiting brackets
...
}).catch(err => console.log(`Error when reporting trace! Error: ${err}`))
...
pavolloffay
force-pushed
the
HWKAPM-798-http-recorder-custom-options
branch
from
a46f216
to
f92fdaf
Compare
|
@pavolloffay I think this came up when you were initially doing the work on the javascript provider? Looking at the fields in the option object I don't see anything that the application should be able to specify, other than possibly the timeout - so not sure it is a good idea to allow application to provide this object. I think allowing control over timeout is a good idea and should be consistent across all languages supporting a HTTP recorder. |
HTTP recorder accepts options object of underlying fetch-node.
pavolloffay
force-pushed
the
HWKAPM-798-http-recorder-custom-options
branch
from
f92fdaf
to
85313dd
Compare
|
PR updated, now it's possible to specify only |
|
@pavolloffay LGTM 👍 |
|
@objectiser @karelhala thanks! |
https://issues.jboss.org/browse/HWKAPM-798
HTTP recorder accepts options object of underlying fetch-node.