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

New post consumer component #52

Merged
merged 8 commits into from Jan 2, 2019

Conversation

@srosset81
Copy link
Collaborator

srosset81 commented Dec 14, 2018

This component POST received data to the configured URL.
Currently only application/json ContentType is accepted.

The name "Webhook" is maybe not right, I wasn't sure how to call it.

Note: this works fine if the "components chain" is in "push-only", for example if you link a "Post provider" component to this component. However if you add a "NoSQL cache" in between, the Webhook component will not be called and thus the data will not be POSTed to the configured URL.

@@ -68,3 +68,6 @@ test/test_result/

#config
configuration.js

#IDE-specific
.idea

This comment has been minimized.

Copy link
@grizio

grizio Dec 22, 2018

Collaborator

Just for your information, you can exclude local-specific files (as IDE or personal directories) in .git/info/exclude file. You can do it for each project or all of the projects of your machine (avoid changing .gitignore of each project you contribute on).

This file already exclude environment-specific files, so do not change here, it is only an information. 🙂

<webhook-editor>
<!-- bouton aide -->
<div class="contenaireH" style="margin-left:97%">
<a href="https://github.com/assemblee-virtuelle/Semantic-Bus/wiki/Composant:-Webhook" target="_blank"><img src="./image/help.png" alt="Aide" width="25px" height="25px"></a>

This comment has been minimized.

Copy link
@grizio

grizio Dec 22, 2018

Collaborator

Aide ? I do not see the bond between aide and webhook.

This comment has been minimized.

Copy link
@srosset81

srosset81 Dec 27, 2018

Author Collaborator

This is the alt tag over the help icon. I've copied what is done in other .tag files.

this.updateData = function (dataToUpdate) {
this.data = dataToUpdate;
this.update();
}.bind(this);

This comment has been minimized.

Copy link
@grizio

grizio Dec 22, 2018

Collaborator

To avoid this bind, you can declare functions as: this.updateData = (dataToUpdate) => {…}.

Also, you use two formats:

urlInputChanged(e) {}
// and
this.updateData = function (dataToUpdate) {}.bind(this);

Is it possible to keep only one of them?

This comment has been minimized.

Copy link
@srosset81

srosset81 Dec 27, 2018

Author Collaborator

Ah I didn't know ES6 arrow functions could be used inside Riot tags.
I've updated the code accordingly.


// Function called when another component push to this component
pull: function(data, flowData, queryParams) {
return new Promise((resolve, reject) => {

This comment has been minimized.

Copy link
@grizio

grizio Dec 22, 2018

Collaborator

You can avoid nested promise construction:

Line 28:

return Promise.reject(new Error('...'));

Line 31+:

       return this.fetch(componentData.url, {
         method: 'POST',
         body: body,
         headers: { 'Content-Type': componentData.contentType }
       }).then(() => ({ data: flowData[0].data }))

Avoiding nested promises reduces memory usage (but not strongly impacting here), enforce composition (.then, .catch) and avoid forgetting cases of termination (here, I do not see one, but if you forgot the .catch, the promise will never end, remains in memory and flood the server).

This comment has been minimized.

Copy link
@srosset81

srosset81 Dec 27, 2018

Author Collaborator

Great idea, it makes the code much more readable. Thanks !

@@ -65,6 +65,7 @@
"mongoose": "^5.3.2",
"mongoose-unique-validator": "^2.0.2",
"mysql": "^2.13.0",
"node-fetch": "^2.3.0",

This comment has been minimized.

Copy link
@grizio

grizio Dec 22, 2018

Collaborator

I believe in this project we use http: https://nodejs.org/api/http.html#http_http_request_options_callback

@simonLouvet : Can you confirm?

This comment has been minimized.

Copy link
@srosset81

srosset81 Dec 27, 2018

Author Collaborator

@grizio I believe the http/https libraries (like the request library) are fairly low-level. When you deal with more complex situations, for example if you are behind a proxy, if you need to handle CORS (?) or redirections, it will require tweaks. (I've seen that for some components, the follow-redirects library is used just to handle the redirections.)

The advantage of node-fetch is that it pretty much handles all cases.

@srosset81

This comment has been minimized.

Copy link
Collaborator Author

srosset81 commented Dec 27, 2018

Thanks for the review @grizio ! I've made some changes and comments.

@srosset81

This comment has been minimized.

Copy link
Collaborator Author

srosset81 commented Dec 27, 2018

@simonLouvet I've improved the code so that, when the webhook POST fails, it retries after 5s, 25s, 2m, 10m, 50m, 4h and 21h.
For the component name, what do you think of "Repost to URL" ?

@srosset81 srosset81 requested a review from simonLouvet Dec 27, 2018
Copy link
Collaborator

grizio left a comment

The component name Webhook seems valid to me. Using a name like repost to url will give the technical behavior of the component, not its business function.

return this.fetch(url, options).catch(error => {
if( numRetry >= 7 ) {
// TODO log the failed webhook posts somewhere ?
console.error(error);

This comment has been minimized.

Copy link
@grizio

grizio Dec 28, 2018

Collaborator

How does the promise end in this case?

This comment has been minimized.

Copy link
@srosset81

srosset81 Jan 2, 2019

Author Collaborator

It ends with no return value.
I don't want to rethrow the error, otherwise Node will stop.

console.log(`Webhook post to ${url} failed, trying again in ${retryInterval}s...`);

return this.sleep(retryInterval * 1000).then(() => {
this.call_webhook(url, options, numRetry+1);

This comment has been minimized.

Copy link
@grizio

grizio Dec 28, 2018

Collaborator

What is the returning value of the promise in this case?

This comment has been minimized.

Copy link
@srosset81

srosset81 Jan 2, 2019

Author Collaborator

Right, it should return the result of this.call_webhook. I've made the correction.

// This will retry after 5s, 25s, 2m, 10m, 50m, 4h, 21h
const retryInterval = Math.pow(5, numRetry+1);

console.log(`Webhook post to ${url} failed, trying again in ${retryInterval}s...`);

This comment has been minimized.

Copy link
@grizio

grizio Dec 28, 2018

Collaborator

The log can be considered as an error or at least as a warning. If some endpoint is failing, we will be alerted only 26h hours later.

This comment has been minimized.

Copy link
@srosset81

srosset81 Jan 2, 2019

Author Collaborator

Yes that's the idea. The URL to call may be down temporarly, IMO this shouldn't be an error.
Do you think we should do it differently ? Should I remove the console.log ?

This comment has been minimized.

Copy link
@grizio

grizio Jan 2, 2019

Collaborator

The log is valid, it is the level I would level up to warning at least.
Maybe not the first times, but if the URL is down 1h (or even 30m), it is already long enough to be informed and maybe to check the issue.

What about this compromise:

  • if numRetry < 4 (for 10m), console.log (like today)
  • else if numRetry > 7 (for 21h), console.error and stop (like today)
  • else (for 10m, 50m and 4h) console.warn (new case)

It could also be a more configurable/smart strategy, I mainly give the idea.

@simonLouvet

This comment has been minimized.

Copy link
Collaborator

simonLouvet commented Dec 28, 2018

@srosset81 name of this component is Post Consumer. @Jacx12 can you create an image for this component. Miror of flow consumer please.

@Jacx12

This comment has been minimized.

@srosset81 srosset81 changed the title New webhook component New post consumer component Jan 2, 2019
@srosset81

This comment has been minimized.

Copy link
Collaborator Author

srosset81 commented Jan 2, 2019

@simonLouvet @grizio I've renamed to Post consumer and made a few corrections.
For me, it's ready to be merged.

@simonLouvet simonLouvet merged commit 2c75a75 into assemblee-virtuelle:master Jan 2, 2019
@simonLouvet

This comment has been minimized.

Copy link
Collaborator

simonLouvet commented Jan 2, 2019

Merged. visible in production at the next Docker Push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.