Closed atc0005 closed 4 years ago
To expand on my earlier notes:
For a local (localhost) app instance, concurrency might not have a large impact, but if run centrally I'm not sure how else this could be reliably handled.
This would likely come as a later, separate feature/enhancement,, but having the user be able to specify:
would allow for creating unique pairs so that you could have multiple groups or individual developers share an instance of this app for testing purposes with each group receiving messages on a separate Teams channel.
Current focus. Once this is implemented here it will help me implement elsewhere where it is a major feature/blocker for development.
To effectively pass relevant details to a Teams channel, I feel like we'll have to extend the MessageCard
type provided by the dasrick/go-teams-notify
package to include additional fields allowed by the legacy MessageCard format.
I first created a truncated/modified JSON payload based on the "Trello - Card created" example JSON from https://messagecardplayground.azurewebsites.net/.
{
"summary": "Summary text here",
"title": "Title goes here",
"text": "Title text goes here",
"themeColor": "#3479BF",
"sections": [
{
"title": "first section",
"text": "first section text here",
"markdown": true
},
{
"title": "Details",
"text": null,
"markdown": true,
"facts": [
{
"name": "Labels",
"value": "Designs, redlines"
},
{
"name": "Due date",
"value": "Dec 7, 2016"
}
]
}
],
"entities": null,
"potentialAction": [
{
"target": [
"http://example.com/report/view/12345"
],
"@context": "http://schema.org",
"@type": "ViewAction",
"@id": null,
"name": "View report",
"isPrimaryAction": false
}
],
"replyTo": null,
"threadingCriteria": null
}
I then used https://mholt.github.io/json-to-go/ to produce this Go struct:
type MessageCardExtended struct {
Summary string `json:"summary,omitempty"`
Title string `json:"title"`
Text string `json:"text"`
ThemeColor string `json:"themeColor"`
Sections []struct {
Title string `json:"title"`
Text string `json:"text"`
Markdown bool `json:"markdown"`
Facts []struct {
Name string `json:"name"`
Value string `json:"value"`
} `json:"facts,omitempty"`
} `json:"sections,omitempty"`
Entities interface{} `json:"entities,omitempty"`
PotentialAction []struct {
Target []string `json:"target"`
Context string `json:"@context"`
Type string `json:"@type"`
ID interface{} `json:"@id"`
Name string `json:"name"`
IsPrimaryAction bool `json:"isPrimaryAction"`
} `json:"potentialAction,omitempty"`
ReplyTo interface{} `json:"replyTo,omitempty"`
ThreadingCriteria interface{} `json:"threadingCriteria,omitempty"`
}
I dropped in some additional omitempty
qualifiers; I plan to refine that struct further to remove anything that doesn't appear to be strictly needed for our purposes. Further (future) work could go in the other direction: add all supported fields to the MessageCard
type in the dasrick/go-teams-notify
package with all non-essential fields marked as omitempty
.
I first created a truncated/modified JSON payload based on the "Trello - Card created" example JSON from https://messagecardplayground.azurewebsites.net/.
I say that, but some of the fields don't appear to be listed on the official reference doc: https://docs.microsoft.com/en-us/outlook/actionable-messages/message-card-reference
Examples:
entities
replyTo
threadingCriteria
I've removed them from the sample I'm working from.
Current sample JSON:
{
"@type": "MessageCard",
"@context": "http://schema.org/extensions",
"summary": "Summary text here",
"title": "Client submission received",
"text": "`GET` request received on `/api/v1/echo/json` endpoint",
"themeColor": "#3479BF",
"sections": [
{
"title": "Received JSON payload",
"text": "```{\"title\":\"testing\"}```",
"markdown": true
},
{
"title": "Individual Go struct fields here",
"text": null,
"markdown": true,
"facts": [
{
"name": "Client IP Address",
"value": "1.2.3.4"
},
{
"name": "Endpoint used",
"value": "/api/v1/echo/json"
}
]
}
],
"potentialAction": [
{
"target": [
"http://web.example.local:8000/app/search/@go?sid=scheduler_admin_search_W2_at_14232356_132"
],
"@context": "http://schema.org",
"@type": "ViewAction",
"@id": null,
"name": "View full Splunk report",
"isPrimaryAction": true
}
]
}
Still refining it. For example, I don't know whether @id
is required under potentialAction
. If not, we can probably leave it out or set it to a unique value that we're perhaps already receiving based on a client request (or we can generate one based off of that event).
Context: Microsoft Teams supports Markdown code formatting via using pairs of ` or ``` characters to wrap code blocks.
Question to self:
Should a failure to submit messages to Microsoft Teams result in a HTTP Status Code change? In other words, if what the client sent was fine and the data was displayed via console output without issue, should we ...
For now I'll refrain from reporting the error sending to Teams via HTTP Status Code changes since sending to Teams is a supplementary action and not currently considered a requirement for this app.
I'll need to give this further thought to see if there is a standard practice around this.
Quick note:
Performance with the way the current codebases are setup is pretty sluggish. Not 100% sure whether it's all of the debug logging or just the delay in getting a message to Teams and coming back with a response.
Current non-concurrent function:
func sendMessage(webhookURL string, msgCard goteamsnotify.MessageCard) error {
if webhookURL == "" {
log.Debug("webhookURL not defined, skipping message submission to Microsoft Teams channel")
}
// Submit message card
if err := send2teams.SendMessage(webhookURL, msgCard); err != nil {
errMsg := fmt.Errorf("ERROR: Failed to submit message to Microsoft Teams: %v", err)
log.Error(errMsg.Error())
return errMsg
}
// Emit basic success message
log.Info("Message successfully sent to Microsoft Teams")
return nil
}
Concurrent, fast yet with a bug version:
func sendMessage(webhookURL string, msgCard goteamsnotify.MessageCard) error {
if webhookURL == "" {
log.Debug("webhookURL not defined, skipping message submission to Microsoft Teams channel")
}
// NOTE: Unscientific testing showed a MASSIVE difference in
// response times when launching this in a goroutine. We'll
// need to find a way to communicate *back* to the caller
// the results of the goroutine, otherwise we are not
// able to properly handle errors.
go func() error {
// Submit message card
if err := send2teams.SendMessage(webhookURL, msgCard); err != nil {
errMsg := fmt.Errorf("ERROR: Failed to submit message to Microsoft Teams: %v", err)
log.Error(errMsg.Error())
return errMsg
}
// Emit basic success message
log.Info("Message successfully sent to Microsoft Teams")
return nil
}()
}
I've not worked with them enough to implement properly, but I would need some reliable way to communicate back the error state to the caller.
Performance with the way the current codebases are setup is pretty sluggish. Not 100% sure whether it's all of the debug logging or just the delay in getting a message to Teams and coming back with a response.
https://blog.simon-frey.eu/manual-flush-golang-http-responsewriter/
For a recent project I wanted to start rendering the HTML page even if the server was still working on a long runing task. (For showing the loading screen of https://unshort.link without the use of JavaScript)
and:
But sadly that did not produce the result I anticipated. The page was still rendered completely at once after the complete process was done. Apparently go buffers there response writer until the handler returns, or the buffer (default 4KB) is full.
The workarounds are to reduce the buffer size (not recommended) or manually flushing the http.ResponseWriter
buffer:
if f, ok := rw.(http.Flusher); ok {
f.Flush()
}
Here is their updated implementation:
func h(rw http.ResponseWriter, req *http.Request) {
io.Copy(rw, loadingHTMLByteReader)
if f, ok := rw.(http.Flusher); ok {
f.Flush()
}
tResult = performLongRunningTask()
rw.Write(tResult)
}
Example before flushing the http.ResponseWriter
buffer:
Example after flushing the http.ResponseWriter
buffer:
Browsing the endpoints via web browser still "feels" a little sluggish, but much, much faster than before. I implemented the buffer flush like so:
writeTemplate := func() {
err := tmpl.Execute(mw, ourResponse)
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
log.Errorf("error occurred while trying to execute template: %v", err)
// We force a a return here since it is unlikely that we should
// execute any other code after failing to generate/write out our
// template
return
}
// Manually flush http.ResponseWriter
// https://blog.simon-frey.eu/manual-flush-golang-http-responsewriter/
if f, ok := w.(http.Flusher); ok {
log.Debug("Manually flushing http.ResponseWriter")
f.Flush()
} else {
log.Warn("http.Flusher interface not available, cannot flush http.ResponseWriter")
log.Warn("Not flushing http.ResponseWriter may cause a noticeable delay between requests")
}
}
The log statements (apex/log
package) are meant to give me a sense of, "it's actually working", but can be disabled later if too much noise is generated.
This function is called from multiple places throughout the echo handler, so adding the flush here where our output template is executed seemed to make the most sense (especially since it is called just before we try sending a message via Teams).
Browsing the endpoints via web browser still "feels" a little sluggish, but much, much faster than before.
Worth noting:
The /api/v1/echo/json
endpoint is very snappy via web browser compared to the /api/v1/echo
endpoint (also via web browser).
My suspicion is that the 405 return code provided by the /api/v1/echo/json
endpoint for GET
requests comes through right away, whereas the other endpoint processes for a bit longer before returning anything (status code or content). Worth reviewing in detail later.
At this point I believe the prototype-use-concurrency-for-notifications-handling
branch has mostly shaped up to what I'm looking for in a v1 implementation for this issue, so I've branched off (again) and created the i21-concurrent-teams-notifications
branch to start cleaning up the (to me) massive pile of unsquashed commits to get a PR into the queue.
As I write this, the emailNotifier
function (used as a background/persistent goroutine) is behind the teamsNotifier
function in equivalency. It is mostly a stub, but the intent was (is?) to keep everything but the stub bits in sync with teamsNotifier
so that when I replaced the stub portions of the code in the function it would be ready to implement as a PR with minimal changes to the surrounding code.
Once the PR for this issue lands I'll go ahead and implement basic SMTP notifications support over unauthenticated 25/tcp (via #19) with the assumption that the primary use case will be sending to internal mail gateways or dev VMs/containers where encryption or tight security isn't required. A follow-up PR could (and probably will) add the additional function to allow sending outside of an isolated network to Gmail, Yahoo, etc, or even to a local corporate mail server where encryption is supported, encouraged or required.
The initial implementation could send each endpoint request over to a specified Microsoft Teams channel, but a later refinement might improve on this by batching the requests for bulk submission, either immediately when hitting a specified queue threshold or at a schedule frequency (if any are in the queue).
The existing
atc0005/send2teams
project code could serve as a starting point for adding this feature.