Skip to content

feat: change generate SVG function#88

Closed
Baoyuantop wants to merge 6 commits intoapi7:masterfrom
Baoyuantop:feat-GenerateSvg
Closed

feat: change generate SVG function#88
Baoyuantop wants to merge 6 commits intoapi7:masterfrom
Baoyuantop:feat-GenerateSvg

Conversation

@Baoyuantop
Copy link
Copy Markdown
Contributor

resolve #87

The current request contributor image API relies on the Google Cloud function, which uses the browser to render the image and return, which makes the speed very slow and the memory usage is large and often crashes.

In order to improve this situation, I made the first modification: modify the cloud function to fetch the required contributor data and then use the data in the node environment to render the chart image with the help of echart. This is helpful in terms of the speed of the development environment.

But there are still some problems that I need to continue to modify. The code in the current PR is obviously immature. I will continue to modify, Please help me review and commit your comments, thanks a lot ~.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Aug 23, 2021

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link
Copy Markdown

vercel Bot commented Aug 23, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/apiseven/contributor-graph/AdWjMQpjqChC1RRouZsAii6cXEup
✅ Preview: https://contributor-graph-git-fork-baoyuantop-feat-generatesvg-apiseven.vercel.app

@Baoyuantop Baoyuantop changed the title feat: change generate SVG function(WIP) feat: change generate SVG function Aug 29, 2021
@Baoyuantop Baoyuantop marked this pull request as ready for review August 29, 2021 14:12
@juzhiyuan juzhiyuan requested a review from yiyiyimu-bot August 29, 2021 14:32
@juzhiyuan
Copy link
Copy Markdown
Contributor

Please take a look if possible 😄 cc @Yiyiyimu

Copy link
Copy Markdown
Contributor

@Yiyiyimu Yiyiyimu left a comment

Choose a reason for hiding this comment

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

Actually I know nothing about Node.js😿, so maybe I need some help here to review that part. Also @LiteSun could you help to make sure the output layout is the same as with the front-end

BTW, the reason we choose to get the graph from the front-end is mainly that we don't want to maintain code for one function in two different places, to increase the burden of maintenance. But considering there is nothing much to change on the front end for now, I do agree to build the graph generation also on the back-end.

Comment thread README.md

[![Monthly Active Contributors](https://contributor-overtime-api.apiseven.com/contributors-svg?chart=contributorMonthlyActivity&repo=apache/apisix&merge=true)](https://www.apiseven.com/en/contributor-graph?chart=contributorMonthlyActivity&repo=apache/apisix&merge=true)

## Development
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.

Since normal contributors could not get access by default, so maybe we could move it to a separate file and leave a link here, so not all people need to see it on README.

Also maybe we could also add guidance on how to get access.

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.

Agree, we do need a guide to tell a new developer how to start this project.

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.

Yes it's pretty complicated, since we deploy it on something we pay for, we need to find the way to give enough permission for users to contribute, but also avoid people abusing it

svg = strings.Replace(svg, "%", "%%", -1)
fmt.Fprintf(w, svg)
} else {
w.Header().Add("content-type", "image/png")
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.

we choose svg but not png before is because svg is relative small (at least 5 times smaller I believe).
Is there any reason to change to png

Copy link
Copy Markdown
Contributor Author

@Baoyuantop Baoyuantop Aug 30, 2021

Choose a reason for hiding this comment

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

I can only produce png when I render the picture in Google Cloud Function. Any better suggestions? thanks

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.

I believe Echarts support both png and svg. I think @LiteSun know more about it

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.

SVG is better, let me check the method again, @LiteSun do you have any good suggestions?

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.

svg would be beteer. see:
image

defer client.Close()

graphFunctionUrl := "https://cloudfunction.contributor-graph.com/svg?repo=" + repo
graphFunctionUrl := "https://asia-east2-api7-301102.cloudfunctions.net/png" + repo
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.

Use the original url, which is actually the local balancer URL of the cloud function which adds google cloud armor

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 don’t know much about Google Cloud, I just want to point it to my newly deployed function.

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.

I think you could bind your new function to the load balancer and there are docs talking about it

return []byte(strings.Join(lines, "\n")), nil
}
}
// svg := string(svgBytes[:])
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.

If we would like to remove this function, we could directly remove it and use git to look for history version

| ---- | ---- | ---- | ---- | ---- |
| repo | true | string | The name of repository | apache/apisix,apache/skywalking |
| merge | false | boolean | Whether to view all repos related to this repo, when chart is `contributorMonthlyActivity`, can not be set true | true |
| chart | false | contributorOverTime contributorMonthlyActivity | chart type | |
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.

something goes wrong here

const { updateSeries } = require('./utils');

const config = {
width: 896,
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.

is there any reason to choose this resolution? Maybe larger graph is better for people to use, especially we encode it as png but not svg

@Baoyuantop
Copy link
Copy Markdown
Contributor Author

Actually I know nothing about Node.js😿, so maybe I need some help here to review that part. Also @LiteSun could you help to make sure the output layout is the same as with the front-end

BTW, the reason we choose to get the graph from the front-end is mainly that we don't want to maintain code for one function in two different places, to increase the burden of maintenance. But considering there is nothing much to change on the front end for now, I do agree to build the graph generation also on the back-end.

At present, this method does cause duplicate code.

@Baoyuantop
Copy link
Copy Markdown
Contributor Author

transfer #96

@Baoyuantop Baoyuantop closed this Aug 31, 2021
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.

Sort through existing API logic and check for "computational" tasks (which can cause request blocking)

5 participants